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

isLiveTable check breaks Kinesis like streams #8052

Closed
npawar opened this issue Jan 21, 2022 · 6 comments
Closed

isLiveTable check breaks Kinesis like streams #8052

npawar opened this issue Jan 21, 2022 · 6 comments

Comments

@npawar
Copy link
Contributor

npawar commented Jan 21, 2022

PR https://github.com/apache/pinot/pull/7756/files introduced a check for liveTable in the path of adding new partitionGroups.
This branch was introduced in setupNewPartitionGroup (prior to this PR we had just the else part)

if (isLiveTable) {
    startOffset = getPartitionGroupSmallestOffset(streamConfig, partitionGroupId);
 } else {
    startOffset = partitionGroupMetadata.getStartOffset();
 }

For new table, we go to the else, and for existing table detecting a new partitionGroup we go to if. Within the if, a call is made to getNewPartitionGroupMetadataList(streamConfig, Collections.emptyList());. For Kinesis like streams, the response from this call depends on the currentList passed. If a Kinesis like stream received empty list, it will only return the very first parent shards. This is because in Kinesis, the shards have a sequence (we started with 0, split it to 1, 2, so 1, 2 will only be returned if the current state tells it that 0 is done ingesting).

As a result of this PR change, no new shards can get detected in Kinesis.

@npawar
Copy link
Contributor Author

npawar commented Jan 21, 2022

Fixing in #8053 by using the patch that was suggested in an alternative PR #7743

Tagging @KKcorps , who detected this bug when enhancing integration tests for Kinesis. The current integration test misses checking this.
Tagging @KKcorps also to take note of this requirement when working on #7058

@npawar
Copy link
Contributor Author

npawar commented Jan 21, 2022

fyi @mcvsubbu @Jackie-Jiang

@Jackie-Jiang
Copy link
Contributor

Have you verified that the approach in #7743 works? I feel it should work since it follows the current way of fetching offsets.
However, the issue for the current approach is that getPartitionGroupSmallestOffset() won't give correct offset for Kinesis, and I think that should be fixed because that is also used to fix the errored consuming segment.

Adding @mcvsubbu @KKcorps for more inputs

@KKcorps
Copy link
Contributor

KKcorps commented Jan 21, 2022

The computePartitionGroupMetadata in KinesisStreamMetadataProvider returns startingSequenceNumber of each shard in case an empty partitionGroupConsumptionStatuses list is passed. This is true for this scenario and hence correct offsets are being returned.

@npawar
Copy link
Contributor Author

npawar commented Jan 21, 2022

Yup 7743 should work as that one doesn't use empty list for current

@npawar
Copy link
Contributor Author

npawar commented Jan 21, 2022

Regarding, getPartitionGroupSmallestOffset(), also agree that this should be fixed regardless for Kinesis as it is used in validation manager. But that's a much smaller and infrequent case, so i think we should prioritize first fixing the bigger case of broken new partitiongroups.

@npawar npawar closed this as completed Feb 1, 2022
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

No branches or pull requests

3 participants