Add null checks in DruidSchema#6830
Conversation
egor-ryashin
left a comment
egor-ryashin
left a comment
There was a problem hiding this comment.
Could you create a unit test which checks if the issue exists?
|
@egor-ryashin I think theoretically, this issue can happen, it'd require some effort to create a unit test for multithreaded case, let me think about how can I add such test. |
@egor-ryashin added unit tests. |
| } | ||
|
|
||
| private void removeSegment(final DataSegment segment) | ||
| protected void removeSegment(final DataSegment segment) |
There was a problem hiding this comment.
Please add @VisibleForTesting to newly exposed methods.
There was a problem hiding this comment.
Yes, forgot to add. Will do.
| @Test | ||
| public void testNullDatasource() throws IOException | ||
| { | ||
| Map<DataSegment, SegmentMetadataHolder> segmentsMetadata = schema.getSegmentMetadata(); |
There was a problem hiding this comment.
segmentMetadatas makes it more clear it's multiple objects.
| final DataSegment segmentToRemove = segments.stream().findFirst().orElse(null); | ||
| Assert.assertFalse(segmentToRemove == null); | ||
| schema.removeSegment(segmentToRemove); | ||
| schema.refreshSegments(segments); // can cause NPE without dataSourceSegments null check in DruidSchema#refreshSegmentsForDataSource |
There was a problem hiding this comment.
It's unclear that we remove the only segment of 'foo2' datasource (and not a segment from foo datasource) as getSegmentMetadata returned Map doesn't guarantee order. Which means it's unclear if we test for dataSourceSegments == null
There was a problem hiding this comment.
yeah, changed to get segment with datasource 'foo2'
clintropolis
left a comment
clintropolis
left a comment
There was a problem hiding this comment.
LGTM 👍
Any more comments @jihoonson?
* Add null checks in DruidSchema * Add unit tests * Add VisibleForTesting annotation * PR comments * unused import
* Add null checks in DruidSchema * Add unit tests * Add VisibleForTesting annotation * PR comments * unused import
5 participants
Fix for #6826