Fix NPE in compactionTask #5613
Conversation
|
I also changed the constructor of |
|
is there a reason why this is no longer included for 0.12.1? |
|
@pdeva we'll backport it |
|
Wait, if there's going to be a backport, then this issue should be milestoned as 0.12.1. @jihoonson, as the author do you think it make sense to backport it? (i.e. does the bug exist in 0.12.0 and is the fix relatively low risk as far as unrelated code is concerned) I'll unmilestone it until a decision is made. |
|
@gianm I think this is worthwhile to backport. I understand your concern about unrelated code changes, but they will be reverted when backporting to 0.12 because unrelated code changes are mostly to make the |
|
If you think it would be better to split this PR into two PRs for fixing NPE in compactionTask and making |
|
It sounds like a good backport candidate to me then. |
There was a problem hiding this comment.
Thanks for the fix Jihoon. You mentioned that you wanted to backport this to 0.12.0 without the Metadata changes (since they aren't compatible) - that sounds good to me.
This PR should be marked 'incompatible' so I will do so. The backport should not be incompatible though.
| @@ -292,9 +302,14 @@ private static DataSchema createDataSchema( | |||
| final List<QueryableIndex> queryableIndices = loadSegments(timelineSegments, segmentFileMap, indexIO); | |||
|
|
|||
| // find merged aggregators | |||
| for (QueryableIndex index : queryableIndices) { | |||
| if (index.getMetadata() == null) { | |||
| throw new RE("Index metadata doesn't exist for interval[%s]", index.getDataInterval()); | |||
There was a problem hiding this comment.
The error message should really include the segment identifier. Imagine the poor user that gets this error and can't figure out which segment to look at.
Perhaps we can keep it along with the QueryableIndex using a Pair or a small static holder class returned from loadSegments.
There was a problem hiding this comment.
Good point. Added segment identifier.
| @@ -304,7 +319,10 @@ private static DataSchema createDataSchema( | |||
|
|
|||
| // find granularity spec | |||
| // set rollup only if rollup is set for all segments | |||
| final boolean rollup = queryableIndices.stream().allMatch(index -> index.getMetadata().isRollup()); | |||
| final boolean rollup = queryableIndices.stream().allMatch(index -> { | |||
| final Boolean isRollup = index.getMetadata().isRollup(); // We have already done null check | |||
There was a problem hiding this comment.
Should probably say // We have already done null check on index.getMetadata() (since Rollup could still be null, and the comment is vague about that)
There was a problem hiding this comment.
Changed to // We have already checked getMetadata() doesn't return null.
| result = 31 * result + (queryGranularity != null ? queryGranularity.hashCode() : 0); | ||
| result = 31 * result + (rollup != null ? rollup.hashCode() : 0); | ||
| return result; | ||
| return Objects.hash(container, Arrays.hashCode(aggregators), timestampSpec, queryGranularity, rollup); |
There was a problem hiding this comment.
equals could be simplified too (using Objects.equals and Arrays.equals)
|
@gianm thanks. Addressed your comments. |
|
Milestoning this as 0.13.0 since not all of it will be backported. The backport should be milestoned as 0.12.1 though. |
|
@jihoonson please go ahead with the backport of non-compatibility-breaking changes. |
* Fix NPE in compactionTask * more annotations for metadata * better error message for empty input * fix build * revert some null checks * address comments
Fixes #5611.
This change is