Refactor logging, introduce JDBC #260
Conversation
| // TODO(@jphui) added for job-gms duplicity debug, throwaway afterwards | ||
|
|
||
| // JDBC sanity check: should MATCH Ebean's results | ||
| if (log.isDebugEnabled() && "com.linkedin.dataJob.azkaban.AzkabanFlowInfo".equals(aspectName)) { |
There was a problem hiding this comment.
I added some comment in the other PR: #265. What is the difference between them?
There was a problem hiding this comment.
I made this change in response to @dnbaker 's suggested changes to use a single SOT for the Aspect name.
The difference:
ModelUtils.getAspectName()=.getCanonicalName()= "get the fully qualified class name".getSimpleName()= "get only the suffix" like 'AzkabanFlowInfo'
There was a problem hiding this comment.
I can move them here to expedite just saw this sorry
dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java
Show resolved
Hide resolved
| return _server.find(EbeanMetadataAspect.class, key); | ||
| } | ||
|
|
||
| // TODO(@jphui) added for job-gms duplicity debug, throwaway afterwards | ||
|
|
||
| // JDBC sanity check: should MATCH Ebean's results |
There was a problem hiding this comment.
how about wrapping this into a method and put it inside the FindMethodology block. Then this can be enabled from the conflg file
There was a problem hiding this comment.
@yangyangv2 The JDBC functionality is intentionally run in parallel alongside the Ebean find() methods. As it is right now, we want them to run simultaneously to cross check results.
Edited the PR description to be more clear.
There was a problem hiding this comment.
I see, then how about moving this logic to line:506, so you don't have to run the same if-check again?
dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java
Show resolved
Hide resolved
| } | ||
| } | ||
| // Encouraged to run AFTER query execution based on getGeneratedSql() documentation | ||
| if (log.isDebugEnabled() && "com.linkedin.dataJob.azkaban.AzkabanFlowInfo".equals(aspectName)) { |
There was a problem hiding this comment.
I would suggest using aspectClass.getSimpleName() to compare with "AzkabanFlowInfo" for performance reason.
how about moving this log into the the method block, so we can control from the config file?
There was a problem hiding this comment.
I moved this out of the block because both DIRECT_SQL and QUERY_BUILDER will run the same logging lines.
Checklist
<refactor / feat> :refactor logging to be more concise and consistent, introduce JDBC query for cross checking with Ebean resultsSuggestions from @dnbaker surrounding logging conventions in #255 have been incorporated. In addition, a JDBC query has been introduced to be used in cross-checking against Ebean's results to determine if the duplicity issue arises from the Ebean layer.
NOTE that JDBC is not added as a 4th Find Methodology; it's intentionally made to be run in parallel in order to allow cross-checking.
The Big Idea
The end result of this PR is that preceding the existing logging we will be able to see the results that JDBC "would have gotten" ... because if BOTH JDBC and Ebean return
nullthen we can be assured that Ebean is not the cuprit!