search

IBMStockTrader / portfolio

Microservice implemented with MicroProfile that persists stock portfolios to JDBC (such as DB2)
Apache License 2.0
7 stars 60 forks source link

Jpa Enhancement #33

Closed andrewdes closed 5 years ago

andrewdes commented 5 years ago

Modify portfolio to use JPA as mentioned here: #22

yeekangc commented 5 years ago

@jwalcorn, please take a look at this PR.

FYI, @gkwan-ibm.

jwalcorn commented 5 years ago

A few comments - not necessarily showstoppers, but things I wanted to discuss before merging:

  1. It looks like the JSON-B POJOs were turned into JPA POJOs. I'd expected separate POJO classes for serialization of JSON on the wire versus for persisting data to the relational database tables. But perhaps that would just be me introducing complexity. Is it OK for a one Portfolio POJO and one Stock POJOs to serve both roles? Maybe yes - just wanted to make sure we're not doing anything in JPA-izing these POJOs that would make them be problematic in their original role as JSON-B POJOs. For example, I also use these POJOs in the clients (like if used in Portfolio, I also copying them into the Trader repo, so it can pass the right JSON, or receive the JSON, to/from Portfolio) - would that be OK (without causing any locks on tables as Trader works on these POJOs, for example)? I think as long as the EntityManager isn't used in Trader (which it wouldn't be), the answer is probably "yes, it's OK".

  2. I want to make sure we aren't changing the API interface here. I don't want to have to change the various clients (like my Java servlet based Trader, and Ryan's Node.js based Tradr, and my Java-based Looper performance/stress client) because of changing something here in the server (like data expected to be passed in, or expected to be returned). I don't think that's happening, but just wanted to confirm. was this tested with the current clients? Said another way - is the wire format unchanged after these changes? And would the OpenAPI for Portfolio be unchanged? The answer needs to be "yes" - and I think it probably is, but just wanted to confirm.

  3. Does there need to be an EntityManager call in submitFeedback after the setFree and setSentiment calls?

  4. Is there any good EntityManager call I could make during a health check? I don't really want to drive SQL calls to DB2 every 10 seconds. But is there some class that gets initialized once, that I could check if it's null or whatever? I think we do that for Mongo in TradeHistory. Not a big deal - just looking if this could help me make my health check code better.

andrewdes commented 5 years ago

Hey @jwalcorn, in response to some of your questions above:

  1. It is ok for them to serve both roles, but I have not tested the example that you gave. I can test it and get back to you on that.

  2. No there should not be any changes to the API interface. Although, I did not test these changes with any of the current clients.

  3. There does not have to be. After getting the portfolio from the getPortfolioWithoutStocks() method it is merged into the current persistence context using em.merge(). The portfolio will then be a managed entity and any changes will be propagated to the database when the transaction is committed. Although, we can call em.flush() after the setFree and setSentiment calls if we want the changes propagated to the database immediately.

  4. Off the top of my head I am not entirely sure. I can look into this further though and update you if I find a clean way to do this.

yeekangc commented 5 years ago

Yes, we shouldn't be changing the API. The externals should stay the same. This should be internal changes only (to use JPA instead of JDBC).

@jwalcorn, if you have a full Stock Trader environment, we can run through additional tests.

jwalcorn commented 5 years ago

FYI, it does result in the following, apparently benign, WARNING message in the pod's log, pretty early during server startup:

{"type":"liberty_message","host":"portfolio-6bdfcfc68b-78zx7","ibm_userDir":"\/opt\/ol\/wlp\/usr\/","ibm_serverName":"defaultServer","message":"CWWJP9991W: [eclipselink.metadata] You have specified multiple ids for the entity class [com.ibm.hybrid.cloud.sample.stocktrader.portfolio.json.Stock] without specifying an @IdClass. By doing this you may lose the ability to find by identity, distributed cache support etc. Note: You may however use EntityManager find operations by passing a list of primary key fields. Else, you will have to use JPQL queries to read your entities. For other id options see @PrimaryKey.","ibm_threadId":"00000036","ibm_datetime":"2019-08-29T18:09:52.456+0000","ibm_messageId":"CWWJP9991W","module":"eclipselink.metadata","loglevel":"WARNING","ibm_sequence":"1567102192456_0000000000049"}