Skip to content

Conversation

@tzulitai
Copy link
Contributor

@tzulitai tzulitai commented Jun 8, 2016

Remove shard list querying from the constructor, and let all subtasks independently discover which shards it should consume from in open(). This change is a prerequisite for FLINK-3231.

Explanation for some changes that might seem irrelevant:

  1. Changed naming of some variables / methods: Since the behaviour of shard assignment to subtasks is now (and will continue to be in the future after FLINK-3231) more like "discovering shards for consuming" instead of "being assigned shards", I've changed the "assignedShards" related namings to "discoveredShards".
  2. I've removed some tests, due to the fact that the corresponding parts of the code will be subject to quite a bit of change with the upcoming changes of FLINK-3231. Tests will be added back with FLINK-3231.

@rmetzger
Copy link
Contributor

rmetzger commented Jun 9, 2016

I'll try to review this change soon.

@tzulitai
Copy link
Contributor Author

tzulitai commented Jun 14, 2016

Hi @rmetzger,
Thanks for letting me know. However, I'd like to close this PR for now for the following reasons:

  1. The new shard-to-subtask assignment logic introduced with this change will actually need to be moved again to run() as part of implementing Kinesis reshard handling (FLINK-3231).
  2. I've tested this change a bit more on Kinesis streams with high shard counts, and it seems like the implementation needs more guarantee on that all subtasks will be able to get the shard list without failing with Amazon's LimitExceededException even after 3 retries. Since the implementation for FLINK-3231 will have a separate thread that polls for changes in the shard list, I'd like to strengthen this guarantee as part of FLINK-3231's PR.

I'm almost done with FLINK-3231, and will reopen a PR to resolve FLINK-3231 and FLINK-4020 together. I'll keep you updated!

@rmetzger
Copy link
Contributor

Okay, thank you. I'll wait then.

@tzulitai
Copy link
Contributor Author

Hi @rmetzger,
Update: I'm closing this PR now. The new PR with FLINK-4020 & FLINK-3231 is at #2131.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants