Fixes Error prone null checks in Pipelines batch submodule #1484
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1484 +/- ##
============================================
- Coverage 46.66% 46.55% -0.11%
- Complexity 676 678 +2
============================================
Files 90 90
Lines 5874 5898 +24
Branches 814 826 +12
============================================
+ Hits 2741 2746 +5
- Misses 2828 2845 +17
- Partials 305 307 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (flinkPipelineMetrics != null && jobClient != null) { | ||
| flinkPipelineMetrics.addJobClient(jobClient); | ||
| } else { | ||
| logger.error("FlinkPipelineMetrics instance or jobClient instance is null"); |
There was a problem hiding this comment.
nit:
| logger.error("FlinkPipelineMetrics instance or jobClient instance is null"); | |
| logger.warn("FlinkPipelineMetrics instance or jobClient instance is null"); |
| } | ||
| } | ||
| totalParseTimeMillisMap.get(resourceType).inc(System.currentTimeMillis() - startTime); | ||
| if (totalParseTimeMillisMap.get(resourceType) != null) |
There was a problem hiding this comment.
nit: consider wrapping these two lines into a function that gets the map, resourceType, and startTime as params, does the null check and then increment. Then call this function everywhere (here and below).
| // class is accessed by a single thread. | ||
| // https://beam.apache.org/documentation/programming-guide/#user-code-thread-compatibility | ||
| private Map<String, Integer> patientCount = null; | ||
| private Map<String, Integer> patientCount = Maps.newHashMap(); |
There was a problem hiding this comment.
I understand that you have added this to silence NullAway warnings but this is a little bit misleading. Because this is a Beam class, the real initialization of this field happens in startBundle on line 117 below. It is fine to keep this initialization but please add a comment about it to prevent future confusion.
There was a problem hiding this comment.
Reverted this to null, added a suppress and a comment.
| private Map<String, Integer> patientCount = Maps.newHashMap(); | |
| //Suppressing warning as this is initialized in StartBundle. | |
| @SuppressWarnings("NullAway.Init") | |
| private Map<String, Integer> patientCount = null; |
|
|
||
| protected AvroConversionUtil avroConversionUtil; | ||
|
|
||
| @SuppressWarnings("NullAway.Init") |
There was a problem hiding this comment.
nit: consider adding a comment why we have this here.
| for (ViewDefinition vDef : views) { | ||
| if (Strings.isNullOrEmpty(vDef.getName())) { | ||
| throw new SQLException("Field `name` in ViewDefinition is not defined."); | ||
| if (views != null) { |
There was a problem hiding this comment.
I think this is one of those examples that it would have made more sense to return an empty list (instead of null) in getViewsForType; and then get rid of many null checks. It would be nice if you make that change.
| numDuplicates.inc(); | ||
| } | ||
| if (numRec > 2) { | ||
| if (numRec > 2 && lastRecord != null && lastRecord.get(ID_KEY) != null) { |
There was a problem hiding this comment.
nit: please add a comment saying that it is guaranteed for lastRecord not to be null if numRec > 1. Also why do we need the lastRecord.get(ID_KEY) != null check? Wouldn't it simply add null as a text to the warn message below?
| @Override | ||
| public void finishBundle(FinishBundleContext context) { | ||
| if (!cachedResources.isEmpty()) { | ||
| if (cachedResources != null && !cachedResources.isEmpty()) { |
There was a problem hiding this comment.
For cachedResources too it is better that we simply initialize it to empty list. In fact, again because this is a Beam related class, this is guaranteed because that's what we do in setup. But anyway to keep NullAway happy we can do the same at the field declaration level too (with a comment about setup).
| Pipeline pipeline = Pipeline.create(options); | ||
| log.info("Merging materialized view {}", viewName); | ||
| ViewDefinition viewDef = viewManager.getViewDefinition(viewName); | ||
| ViewDefinition viewDef = viewManager != null ? viewManager.getViewDefinition(viewName) : null; |
There was a problem hiding this comment.
Hmm, I think this is one of those cases that viewManager is guaranteed not to be null here (you can see it in the logic of the above function when this is called). So I would prefer that we change the signature and remove @Nullable from viewManager, then deal with it in the caller function above.
3 participants
Description of what I changed
Ticket #1474
E2E test
TESTED:
Please replace this with a description of how you tested your PR beyond the
automated e2e/unit tests.
Checklist: I completed these to help reviewers :)
I have read and will follow the
review process.
I am familiar with Google Style Guides for the language I have coded in.
No? Please take some time and review
Java and
Python style guides.
My IDE is configured to follow the Google
code styles.
No? Unsure? ->
configure your IDE.
I have added tests to cover my changes. (If you refactored existing
code that was well tested you do not have to add tests)
I ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
If I made any Python code changes, I ran
black .andpylint .rightbefore creating this pull request and added all formatting changes to my
commit.
All new and existing tests passed.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master