[FLINK-10342] Filter restored partitions with discovered partitions b… #7726

fengli · 2019-02-17T22:00:38Z

What is the purpose of the change

Filter restored partitions with current discovered partitions. It's enabled by default since it'll result unexpected behaviors - e.g. When changing the topic name, or remove some topics, The removed/renamed partitions will be still consumed.

Brief change log

Filter restored partitions with current discovered partitions
Add disableFilterRestoredPartitionsWithDiscovered to disable the default behavior if necessary

Verifying this change

This change added tests and can be verified as follows:

Added UTest

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (JavaDocs )

flinkbot · 2019-02-17T22:01:46Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

✅ 1. The [description] looks good.
- Approved by @dawidwys [committer]
✅ 2. There is [consensus] that the contribution should go into to Flink.
- Approved by @dawidwys [committer]
❓ 3. Needs [attention] from.
✅ 4. The change fits into the overall [architecture].
- Approved by @dawidwys [committer]
✅ 5. Overall code [quality] is good.
- Approved by @tzulitai [PMC]

Please see the Pull Request Review Guide for a full explanation of the review process.

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve the 1st aspect (similarly, it also supports the consensus, architecture and quality keywords)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval

fengli · 2019-02-17T22:04:53Z

cc @tzulitai

...a-base/src/main/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBase.java

fengli · 2019-02-21T14:16:10Z

@tzulitai Do you think you can review this small patch? (Do let me know if there are still prior steps before the review process).

dawidwys · 2019-02-25T12:31:24Z

@flinkbot approve description
@flinkbot approve consensus
@flinkbot approve architecture

tzulitai

Hi @fengli, thank you for the contribution.
I sincerely apologize for the bumpy contribution experience you had so far. I've been hard blocked on some Flink 1.8.0 development work, and didn't find time to look at this until now.

Please let me know what you think about the comment, most importantly whether or not we should really be filtering based on external system returned results.

...a-base/src/main/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBase.java

tzulitai · 2019-02-25T15:57:07Z

...se/src/test/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBaseTest.java

+	 */
+	@Test
+	public void testSetFilterRestoredParitionsNoChange() throws Exception {
+		checkFilterRestoredPartitionsWithDisovered(Arrays.asList(new String[]{"kafka_topic_1", "kafka_topic_2"}),


nit: I would also let the first argument be a new line, just for consistency and readability.
Same for the other tests.

tzulitai · 2019-02-25T16:27:52Z

@flinkbot approve quality

tzulitai · 2019-02-26T02:18:39Z

@fengli one comment about addressing reviews:
Unless necessary, I suggest to only push follow-up commits to address review comments to retain the history of the PR, and do not force-push / squash commits yet.
If you always squash and force-push for every review pass, it may be hard for the reviewer to follow the changes. It might be ok for smaller PRs like this one, but once the PR gets bigger and reviewers only revisits the PR after a few days, it will get tough to recall what happened in past reviews.

…y default.

…erer).

...a-base/src/main/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBase.java

tzulitai · 2019-02-27T03:50:16Z

There seems to be failing tests that are related:

17:52:59.921 [ERROR] Failures: 
17:52:59.921 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestoreFromEmptyStateWithPartitions:263
17:52:59.921 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestoreFromEmptyStateWithPartitions:263
17:52:59.921 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestoreFromEmptyStateWithPartitions:263
17:52:59.921 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestoreFromEmptyStateWithPartitions:263
17:52:59.921 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestoreFromEmptyStateWithPartitions:263
17:52:59.921 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestoreFromEmptyStateWithPartitions:263
17:52:59.921 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestore:307
17:52:59.921 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestore:307
17:52:59.923 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestore:307
17:52:59.923 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestore:307
17:52:59.923 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestore:307
17:52:59.923 [ERROR]   FlinkKafkaConsumerBaseMigrationTest.testRestore:307

tzulitai · 2019-02-27T09:30:11Z

This looks good to merge now!
Thanks a lot for the contribution @fengli.

I'll proceed to clean this PR up and merge this for 1.8.0 ..

…titionsWithSubscribedTopics method in FlinkKafkaConsumerBase This closes apache#7726.

…titionsWithSubscribedTopics method in FlinkKafkaConsumerBase This closes #7726.

…titionsWithSubscribedTopics method in FlinkKafkaConsumerBase This closes apache#7726.

klion26 reviewed Feb 18, 2019

View reviewed changes

...a-base/src/main/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBase.java Show resolved Hide resolved

rmetzger added the review=quality? label Feb 25, 2019

tzulitai reviewed Feb 25, 2019

View reviewed changes

rmetzger added review=approved ✅ and removed review=quality? labels Feb 25, 2019

fengli force-pushed the FLINK-10342 branch 2 times, most recently from a1485dd to 0161094 Compare February 25, 2019 22:20

Feng LI and others added 3 commits February 26, 2019 13:56

[FLINK-10342] Added isMatchingTopic to KafkaTopicsDescriptor

6ab85ad

[FLINK-10342] Filter restored partitions with discovered partitions b…

2ebb589

…y default.

Use topicsDescriptor to filter partitions (instead of PartitionDiscov…

b6cd043

…erer).

fengli force-pushed the FLINK-10342 branch from 0161094 to b6cd043 Compare February 26, 2019 13:23

tzulitai reviewed Feb 26, 2019

View reviewed changes

...a-base/src/main/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaConsumerBase.java Outdated Show resolved Hide resolved

Feng LI added 4 commits February 26, 2019 15:51

Resolve conflicts.

d523b0e

Add licence for KafkaTopicsDescriptorTest.

a8dc303

remove import java.util.*

d65dfb9

Fix checkstyle errors.

3e6964c

Feng LI added 3 commits February 27, 2019 09:16

Fix tests in FlinkKafkaConsumerBaseMigrationTest.

e29f28f

Remove unused import.

6bfced6

Create TOPICS as class level constant.

c994aad

tzulitai added a commit to tzulitai/flink that referenced this pull request Feb 28, 2019

[FLINK-10342] [kafka] Improve Javadoc of new disableFilterRestoredPar…

a9f438f

…titionsWithSubscribedTopics method in FlinkKafkaConsumerBase This closes apache#7726.

tzulitai added a commit to tzulitai/flink that referenced this pull request Feb 28, 2019

[FLINK-10342] [kafka] Improve Javadoc of new disableFilterRestoredPar…

c3502e7

…titionsWithSubscribedTopics method in FlinkKafkaConsumerBase This closes apache#7726.

tzulitai added a commit to tzulitai/flink that referenced this pull request Feb 28, 2019

[FLINK-10342] [kafka] Improve Javadoc of new disableFilterRestoredPar…

44e438b

…titionsWithSubscribedTopics method in FlinkKafkaConsumerBase This closes apache#7726.

asfgit closed this in 6a4f012 Feb 28, 2019

asfgit pushed a commit that referenced this pull request Feb 28, 2019

[FLINK-10342] [kafka] Improve Javadoc of new disableFilterRestoredPar…

572a57c

…titionsWithSubscribedTopics method in FlinkKafkaConsumerBase This closes #7726.

fengli deleted the FLINK-10342 branch February 28, 2019 13:08

rmetzger added the component=Connectors/Kafka label Mar 18, 2019

HuangZhenQiu pushed a commit to HuangZhenQiu/flink that referenced this pull request Apr 22, 2019

[FLINK-10342] [kafka] Improve Javadoc of new disableFilterRestoredPar…

a9fe0c4

…titionsWithSubscribedTopics method in FlinkKafkaConsumerBase This closes apache#7726.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-10342] Filter restored partitions with discovered partitions b… #7726

[FLINK-10342] Filter restored partitions with discovered partitions b… #7726

fengli commented Feb 17, 2019

flinkbot commented Feb 17, 2019 •

edited

Loading

fengli commented Feb 17, 2019

fengli commented Feb 21, 2019

dawidwys commented Feb 25, 2019

tzulitai left a comment

tzulitai Feb 25, 2019

fengli Feb 25, 2019

tzulitai commented Feb 25, 2019

tzulitai commented Feb 26, 2019

tzulitai commented Feb 27, 2019

tzulitai commented Feb 27, 2019

[FLINK-10342] Filter restored partitions with discovered partitions b… #7726

[FLINK-10342] Filter restored partitions with discovered partitions b… #7726

Conversation

fengli commented Feb 17, 2019

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Feb 17, 2019 • edited Loading

Review Progress

fengli commented Feb 17, 2019

fengli commented Feb 21, 2019

dawidwys commented Feb 25, 2019

tzulitai left a comment

Choose a reason for hiding this comment

tzulitai Feb 25, 2019

Choose a reason for hiding this comment

fengli Feb 25, 2019

Choose a reason for hiding this comment

tzulitai commented Feb 25, 2019

tzulitai commented Feb 26, 2019

tzulitai commented Feb 27, 2019

tzulitai commented Feb 27, 2019

flinkbot commented Feb 17, 2019 •

edited

Loading