STORM-3082 Add support to handle absent topics #2755
Conversation
aniketalhat
changed the title
|
@aniketalhat I think you are targeting the wrong branch. You should target the branch you started working on (e.g. 1.x-branch). We can review 1.x first, but we'll also need a PR against master at some point before we can merge this. |
|
I guess this should target 1.0.x-branch, going by the listed commits. |
|
@srdo I updated the base as per your suggestions, thank you |
|
|
||
| public SimpleConsumer register(Broker host, String topic, int partition) { | ||
| if (!_connections.containsKey(host)) { | ||
|
|
There was a problem hiding this comment.
Not sure what happened here
There was a problem hiding this comment.
Me neither. You can just revert the file and this change should go away.
srdo
left a comment
srdo
left a comment
There was a problem hiding this comment.
Looks good, only have a few comments. Once the changes are done, please squash to one commit. At that point we can try cherry-picking these changes to master.
| for (String topic : topics) { | ||
| for (PartitionInfo partitionInfo: consumer.partitionsFor(topic)) { | ||
| allPartitions.add(new TopicPartition(partitionInfo.topic(), partitionInfo.partition())); | ||
| if(consumer.partitionsFor(topic) != null) { |
There was a problem hiding this comment.
I think it would be better to put the result of partitionsFor in a variable, rather than asking for it twice
There was a problem hiding this comment.
Whether it inlines the method or not, the code is still executed twice. Besides, extracting to a variable means we don't have to worry about stuff like someone making a change to one call and not another, and it makes the code less noisy.
|
|
||
| List<TopicPartition> presentPartitions = filter.getFilteredTopicPartitions(consumerMock); | ||
| assertThat("Expected filter to pass only topics which are present", presentPartitions, | ||
| containsInAnyOrder(new TopicPartition(presentTopic, 2))); |
There was a problem hiding this comment.
Nit: When there is only one entry, you don't have to specify "in any order" :)
|
|
||
| public SimpleConsumer register(Broker host, String topic, int partition) { | ||
| if (!_connections.containsKey(host)) { | ||
|
|
There was a problem hiding this comment.
Me neither. You can just revert the file and this change should go away.
aniketalhat
force-pushed
the
feature/STORM-3082-Add-support-to-handle-absent-topics
branch
from
c699a3c to
0cacb10
Compare
|
While this PR is based on 1.0.x-branch since the branch is based on that branch, I guess we wouldn't want to maintain 1.0.x-branch any more. We would be better to cherry pick squashed commit into active version lines other than merging this PR itself. |
|
Oops, yes. Jungtaek is right, I don't think we're working on 1.0.x anymore. The changes look good to me. +1. I'll try cherry-picking these changes to master, 1.x and 1.1.x in a bit. |
|
@aniketalhat Thanks for the fix. I cherry picked the changes to master, 1.x and 1.1.x. Could you please close this PR, as the changes have been merged? |
4 participants
No description provided.