Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the realtime time creation unit test #6032

Merged
merged 1 commit into from Sep 18, 2020

Conversation

snleee
Copy link
Contributor

@snleee snleee commented Sep 17, 2020

  1. Added the precondition check for time interval from segment metadata
  2. Added the unit test to check the case when the invalid time column name
    is configured in the table config

@snleee snleee force-pushed the improve-realtime-table-creation branch from 91e1658 to 295371e Compare September 17, 2020 23:39
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. While seems the validation is not related to the unit test?

@snleee
Copy link
Contributor Author

snleee commented Sep 17, 2020

@Jackie-Jiang I will rename the description and title.

1. Added the precondition check for time interval from segment metadata
2. Added the unit test to check the case when the invalid time column name
   is configured in the table config
@snleee snleee force-pushed the improve-realtime-table-creation branch from 295371e to cb5e34e Compare September 17, 2020 23:53
@snleee snleee changed the title Improve the realtime time column validation Improve the realtime time creation unit test Sep 17, 2020
@@ -504,6 +504,8 @@ private LLCRealtimeSegmentZKMetadata updateCommittingSegmentZKMetadata(String re
committingSegmentZKMetadata.setDownloadUrl(isPeerURL(committingSegmentDescriptor.getSegmentLocation())
? CommonConstants.Segment.METADATA_URI_FOR_PEER_DOWNLOAD : committingSegmentDescriptor.getSegmentLocation());
committingSegmentZKMetadata.setCrc(Long.valueOf(segmentMetadata.getCrc()));
Preconditions.checkNotNull(segmentMetadata.getTimeInterval(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consumption will stop if this condition hits. Are we sure that is what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcvsubbu This condition should not happen after the fix from #5966. When this happens, the realtime segment commit would anyway fail and the consumption would be stopped due to the null pointer exception. (egmentMetadata.getTimeInterval().getStartMillis() gets called at the following line). I'm just trying to add more information to the logs here since the log message was not informative when we hit that.

We recently saw the null pointer error issue (and the consumption actually stopped) when the time column is misconfigured.

2020/09/17 20:36:52.666 ERROR [SegmentCompletionFSM_XXXXX__3__0__20200915T2142Z] [grizzly-http-server-3] [pinot-controller] [] Caught exception while committing segment metadata for segment: XXXXX__3__0__20200915T2142Z
java.lang.NullPointerException: null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants