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

[issues 5698]Handle replicator producer as generated name producer #5701

Merged
merged 3 commits into from Nov 21, 2019

Conversation

weishuisheng
Copy link
Contributor

@weishuisheng weishuisheng commented Nov 19, 2019

Fixes #5698

Motivation

Since #5571 handle the generated producer name, the replicator producer was created by broker, only one producer for a replicated topic.

So, we can handle it simple by considered replicator producer as generated name producer.

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (yno)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, just left some comments about the code format

@@ -316,6 +316,11 @@ private void tryOverwriteOldProducer(Producer oldProducer, Producer newProducer)
}
}

private boolean isUserProvidedProducerName(Producer producer){
//considered replicator producer as generated name producer
return producer.isUserProvidedProducerName() && !producer.getProducerName().startsWith(replicatorPrefix);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems 2 space here, please keep 1 space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

topic.removeProducer(producer4);
Assert.assertEquals(topic.getProducers().size(), 0);

Producer producer5= new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add space before =, please also check other places

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@codelipenghui
Copy link
Contributor

run java8 tests

topic.removeProducer(producer4);
Assert.assertEquals(topic.getProducers().size(), 0);

Producer producer5= new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Producer producer5= new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",
Producer producer5 = new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

topic.addProducer(producer5);
Assert.assertEquals(topic.getProducers().size(), 1);

Producer producer6= new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Producer producer6= new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",
Producer producer6 = new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK


topic.getProducers().values().forEach(producer -> Assert.assertEquals(producer.getEpoch(), 2));

Producer producer7= new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Producer producer7= new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",
Producer producer7 = new Producer(topic, serverCnx, 2 /* producer id */, "pulsar.repl.cluster1",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@@ -316,6 +316,11 @@ private void tryOverwriteOldProducer(Producer oldProducer, Producer newProducer)
}
}

private boolean isUserProvidedProducerName(Producer producer){
//considered replicator producer as generated name producer
return producer.isUserProvidedProducerName() && !producer.getProducerName().startsWith(replicatorPrefix);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return producer.isUserProvidedProducerName() && !producer.getProducerName().startsWith(replicatorPrefix);
return producer.isUserProvidedProducerName() && !producer.getProducerName().startsWith(replicatorPrefix);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@codelipenghui
Copy link
Contributor

run java8 tests
run integration tests

@codelipenghui
Copy link
Contributor

run java8 tests

@codelipenghui
Copy link
Contributor

run integration tests

@jiazhai jiazhai merged commit 39f37c9 into apache:master Nov 21, 2019
@sijie sijie modified the milestones: 2.4.3, 2.5.0 Jan 22, 2020
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.

Geo replicator has ProducerBusyException
4 participants