Conversation
|
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
reason: valueType is never changed
There was a problem hiding this comment.
reason: node is used before, cannot be null here
There was a problem hiding this comment.
I wonder if that use is actually a bug....
There was a problem hiding this comment.
Good point. That use is nodeId = node.getId() in the line above. If this use shouldn't be there, how would we get the nodeId for the NoSuchNodeException that is thrown right in this block?
There was a problem hiding this comment.
@bleskes maybe you are familiar enough with this code to be able to judge whether node can be null here at all? From what I see so far, the nodes array is retrieved earlier in the method through request.concreteNodes(). Following the call hierarchy it is unlikely this can contain null references, but I don't think it is enforced anywhere. Then again, the reference in L179 hasn't led to NPEs since at least mid-2016 when it was added, so maybe its save to assume node cannot be null here.
There was a problem hiding this comment.
+1 remove the node == null check. IMO if we pass a null in there it's bug higher up in the chain which we should learn about and fix where it came from.
There was a problem hiding this comment.
reason: shapeType is tested for null earlier, throws exception already there
There was a problem hiding this comment.
reason: md is used in L1477 already
There was a problem hiding this comment.
L70-73 shows up as dead, but from the comment on the constant it appears this is used occassionaly, so maybe supressing and leaving a comment is a good idea.
There was a problem hiding this comment.
Instead of having this constant, could the if condition be commented out with a comment it should be uncommented when needed for debugging? If code must be changed to enable this, then it seems a constant isn't really helping anything here, and adding the unused suppression adds error room for other bugs.
There was a problem hiding this comment.
@droberts195 can probably comment. As far as I see the CATEGORIZATION_TOKENIZATION_IN_JAVA is used in several places throughout the ML test code, but needs to be enabled manually. I thing supressing the warning and commenting here is sufficient
There was a problem hiding this comment.
Yes, it's used in 9 places. The alternative would be to get rid of the constant and comment out the code in the 9 places, but then somebody who wanted to run the test would have to remember all 9 places to uncomment. It's nice to just be able to change 1 constant.
I guess the scope of suppression could be minimised by adding a new method writeCategorizationFilters and just applying the suppression of warnings to that new method. writeScheduledEvents is already broken into a separate method that checks a (different) condition and maybe writes one piece of config.
Removing some dead code or supressing warnings where apropriate. Most of the time the variable tested for null is dereferenced earlier or never used before.
|
Thanks everybody, would anybody care to do a final review/approval of these changes? |
* master: Remove reference to non-existent store type (#32418) [TEST] Mute failing FlushIT test Fix ordering of bootstrap checks in docs (#32417) [TEST] Mute failing InternalEngineTests#testSeqNoAndCheckpoints [TEST] Mute failing testConvertLongHexError bump lucene version after backport Upgrade to Lucene-7.5.0-snapshot-608f0277b0 (#32390) [Kerberos] Avoid vagrant update on precommit (#32416) TESTS: Move netty leak detection to paranoid level (#32354) [DOCS] Fixes formatting of scope object in job resource Copy missing segment attributes in getSegmentInfo (#32396) AbstractQueryTestCase should run without type less often (#28936) INGEST: Fix Deprecation Warning in Script Proc. (#32407) Switch x-pack/plugin to new style Requests (#32327) Docs: Correcting a typo in tophits (#32359) Build: Stop double generating buildSrc pom (#32408) TEST: Avoid triggering merges in FlushIT Fix missing JavaDoc for @throws in several places in KerberosTicketValidator. Switch x-pack full restart to new style Requests (#32294) Release requests in cors handler (#32364) Painless: Clean Up PainlessClass Variables (#32380) Docs: Fix callouts in put license HL REST docs (#32363) [ML] Consistent pattern for strict/lenient parser names (#32399) Update update-settings.asciidoc (#31378) Remove some dead code (#31993) Introduce index store plugins (#32375) Rank-Eval: Reduce scope of an unchecked supression Make sure _forcemerge respects `max_num_segments`. (#32291) TESTS: Fix Buf Leaks in HttpReadWriteHandlerTests (#32377) Only enforce password hashing check if FIPS enabled (#32383)
Removing some dead code or supressing warnings where apropriate. Most of the
time the variable tested for null is dereferenced earlier or never used before.