You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2023/07/26 15:44:15 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #20888: [fix] [broker] In replication scenario, remote consumer could not be registered if there has no message was sent
poorbarcode opened a new pull request, #20888:
URL: https://github.com/apache/pulsar/pull/20888
### Motivation
In the replication scenario, we want to produce messages on the native cluster and consume messages on the remote cluster, the producer and consumer both use a same schema, but the consumer cannot be registered if there has no messages in the topic yet.
The root cause is that for the remote cluster, there is a producer who has been registered with `AUTO_PRODUCE_BYTES` schema, so there is no schema to check the compatibility.
### Modifications
If there is no schema and only the replicator producer was registered, skip the compatibility check.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode merged pull request #20888: [fix][broker] In replication scenario, remote consumer could not be registered if there has no message was sent
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode merged PR #20888:
URL: https://github.com/apache/pulsar/pull/20888
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #20888: [fix] [broker] In replication scenario, remote consumer could not be registered if there has no message was sent
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #20888:
URL: https://github.com/apache/pulsar/pull/20888#discussion_r1275715909
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/OneWayReplicatorTest.java:
##########
@@ -75,4 +79,29 @@ public void testReplicatorProducerStatInTopic() throws Exception {
admin2.topics().delete(topicName);
});
}
+
+ @Test
+ public void testCreateRemoteConsumerFirst() throws Exception {
+ final String topicName = BrokerTestUtil.newUniqueName("persistent://" + defaultNamespace + "/tp_");
+ Producer<String> producer1 = client1.newProducer(Schema.STRING).topic(topicName).create();
+ // Wait for replicator started.
+ Awaitility.await().untilAsserted(() -> {
+ Optional<Topic> topicOptional2 = pulsar2.getBrokerService().getTopic(topicName, false).get();
+ assertTrue(topicOptional2.isPresent());
+ PersistentTopic persistentTopic2 = (PersistentTopic) topicOptional2.get();
+ assertTrue(!persistentTopic2.getProducers().isEmpty());
Review Comment:
```suggestion
assertFalse(persistentTopic2.getProducers().isEmpty());
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java:
##########
@@ -1290,6 +1290,33 @@ private void testIncompatibleSchema() throws Exception {
assertThrows(SchemaSerializationException.class, message2::getValue);
}
+ /**
+ * This test just ensure that schema check still keeps the original logic: if there has any producer, but no schema
+ * was registered, the new consumer could not register new schema.
+ * TODO: I think this design should be improved: if a producer used "AUTO_PRODUCE_BYTES" schema, we should allow
+ * the new consumer to register new schema. But before we can solve this problem, we need to modify
+ * "CmdProducer" to let the Broker know that the Producer uses a schema of type "AUTO_PRODUCE_BYTES".
Review Comment:
Yes, I agree. It should be the right solution. Please help create an issue to track the following fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #20888: [fix] [broker] In replication scenario, remote consumer could not be registered if there has no message was sent
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20888:
URL: https://github.com/apache/pulsar/pull/20888#discussion_r1275742841
##########
pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java:
##########
@@ -1290,6 +1290,33 @@ private void testIncompatibleSchema() throws Exception {
assertThrows(SchemaSerializationException.class, message2::getValue);
}
+ /**
+ * This test just ensure that schema check still keeps the original logic: if there has any producer, but no schema
+ * was registered, the new consumer could not register new schema.
+ * TODO: I think this design should be improved: if a producer used "AUTO_PRODUCE_BYTES" schema, we should allow
+ * the new consumer to register new schema. But before we can solve this problem, we need to modify
+ * "CmdProducer" to let the Broker know that the Producer uses a schema of type "AUTO_PRODUCE_BYTES".
Review Comment:
Done, it is https://github.com/apache/pulsar/issues/20891
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #20888: [fix] [broker] In replication scenario, remote consumer could not be registered if there has no message was sent
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20888:
URL: https://github.com/apache/pulsar/pull/20888#issuecomment-1653927462
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/20888?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#20888](https://app.codecov.io/gh/apache/pulsar/pull/20888?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b8425c1) into [master](https://app.codecov.io/gh/apache/pulsar/commit/ffb9d303f85cea0abd0839acb1cdcbddcc460b1d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ffb9d30) will **increase** coverage by `38.40%`.
> Report is 1 commits behind head on master.
> The diff coverage is `100.00%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/20888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #20888 +/- ##
=============================================
+ Coverage 34.70% 73.11% +38.40%
- Complexity 12078 32226 +20148
=============================================
Files 1697 1874 +177
Lines 129786 139377 +9591
Branches 14157 15328 +1171
=============================================
+ Hits 45037 101899 +56862
+ Misses 78757 29402 -49355
- Partials 5992 8076 +2084
```
| Flag | Coverage Δ | |
|---|---|---|
| inttests | `24.21% <100.00%> (?)` | |
| systests | `25.09% <100.00%> (+<0.01%)` | :arrow_up: |
| unittests | `72.39% <100.00%> (+40.34%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/pulsar/pull/20888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...sar/broker/service/persistent/PersistentTopic.java](https://app.codecov.io/gh/apache/pulsar/pull/20888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudFRvcGljLmphdmE=) | `79.17% <ø> (+28.78%)` | :arrow_up: |
| [...rg/apache/pulsar/broker/service/AbstractTopic.java](https://app.codecov.io/gh/apache/pulsar/pull/20888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0VG9waWMuamF2YQ==) | `86.45% <100.00%> (+28.24%)` | :arrow_up: |
... and [1454 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/20888/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org