You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "vineeth1995 (via GitHub)" <gi...@apache.org> on 2023/02/22 18:24:10 UTC
[GitHub] [pulsar] vineeth1995 opened a new pull request, #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
vineeth1995 opened a new pull request, #19605:
URL: https://github.com/apache/pulsar/pull/19605
### Motivation
This completes https://github.com/apache/pulsar/issues/16551 and extension to this part-1 pr https://github.com/apache/pulsar/pull/17962
This handles Replicator and message ordering guarantee part for blue-green deployment.
**Replicator and message ordering handling**
A. Incoming replication messages from other region's replicator producers to Blue cluster
This will not impact ordering messages coming from the other regions to blue/green cluster. After marking blue cluster, blue cluster will reject replication writes from remote regions and redirects remote producers to the Green cluster where new messages will be written. Consumers of Blue clusters will only be redirected to green once they received all messages from blue. So, migration gives an ordering guarantee for messages replicating from remote regions.
B. Outgoing replication messages from Blue cluster's replicator producers to other regions
The broker can give an ordering guarantee in this case with the trade-off of topic unavailability until the blue cluster replicates all existing published messages in the blue cluster before the topic gets terminated.
Blue cluster marks topic terminated and migrated
Topic will not redirect producers/consumers until all the replicators reaches end of topic and replicates all messages to remote regions. Topic will send TOPIC_UNAVAILABLE message to producers/consumers so, they can keep retrying until replicators reach to end of topics.
Broker disconnects all the replicators and delete them once they reach end of topic.
Broker start sending migrated-command to producer/consumers to redirect clients to green cluster.
### Modifications
- Handle producers so that message ordering is guaranteed when topic has been migrated but replication backlog still exists.
Example use case:
producer1 sends messages msg1, msg2 -> region1
region1 replicator -> msg1 ->region2
but region2 has a connectivity issue with region1
as a result region1 has a replication backlog msg2 with region2
Marked blue-green
region1 -> region1A
If you redirect producer1 to region1A
producer1 sends msg3 to region1A
region1A is connected to region2
region1A sends msg3 to region2
Meanwhile if region1 gets it's connection back to region2
region1 sends msg2(replication backlog) to region2
region2 consumer consumes in the order msg1, msg3, msg2
which is a wrong order of messages as it should be msg1, msg2, msg3
----------
So we don't want to redirect producer1 until Replicator has no backlog. This pr handles this use case by making sure replication backlog is drained before redirecting the producers to green cluster.
### Verifying this change
Added end t end test to verify this change.
### Does this pull request potentially affect one of the following parts:
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [x] `doc-complete` <!-- Docs have been already added -->
--
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] vineeth1995 commented on pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "vineeth1995 (via GitHub)" <gi...@apache.org>.
vineeth1995 commented on PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#issuecomment-1501227302
/pulsarbot run-failure-checks
--
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] michaeljmarshall commented on a diff in pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#discussion_r1117616587
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1574,13 +1575,19 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
if (ex.getCause() instanceof BrokerServiceException.TopicMigratedException) {
Optional<ClusterUrl> clusterURL = getMigratedClusterUrl(service.getPulsar());
if (clusterURL.isPresent()) {
- log.info("[{}] redirect migrated producer to topic {}: producerId={}, {}", remoteAddress, topicName,
- producerId, ex.getCause().getMessage());
- commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
- clusterURL.get().getBrokerServiceUrl(), clusterURL.get().getBrokerServiceUrlTls());
- closeProducer(producer);
- return null;
-
+ if (topic.isReplicationBacklogExist()) {
+ log.info("Topic {} is migrated but replication backlog exist: "
+ + "producerId = {}, producerName = {}, {}", topicName,
+ producerId, producerName, ex.getCause().getMessage());
+ } else {
+ log.info("[{}] redirect migrated producer to topic {}: "
+ + "producerId={}, producerName = {}, {}", remoteAddress,
+ topicName, producerId, producerName, ex.getCause().getMessage());
+ commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
Review Comment:
I'm so sorry @vineeth1995, I didn't realize that `commandSender.sendTopicMigrated` was doing that version check. I think it makes sense to log the error as you just did, but do we need to consider sending an unretriable error to the client to make sure they do not constantly attempt to reconnect?
--
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] vineeth1995 commented on pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "vineeth1995 (via GitHub)" <gi...@apache.org>.
vineeth1995 commented on PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#issuecomment-1478713403
/pulsarbot run-failure-checks
--
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] michaeljmarshall commented on a diff in pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#discussion_r1116498630
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1574,13 +1575,19 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
if (ex.getCause() instanceof BrokerServiceException.TopicMigratedException) {
Optional<ClusterUrl> clusterURL = getMigratedClusterUrl(service.getPulsar());
if (clusterURL.isPresent()) {
- log.info("[{}] redirect migrated producer to topic {}: producerId={}, {}", remoteAddress, topicName,
- producerId, ex.getCause().getMessage());
- commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
- clusterURL.get().getBrokerServiceUrl(), clusterURL.get().getBrokerServiceUrlTls());
- closeProducer(producer);
- return null;
-
+ if (topic.isReplicationBacklogExist()) {
+ log.info("Topic {} is migrated but replication backlog exist: "
+ + "producerId = {}, producerName = {}, {}", topicName,
+ producerId, producerName, ex.getCause().getMessage());
+ } else {
+ log.info("[{}] redirect migrated producer to topic {}: "
+ + "producerId={}, producerName = {}, {}", remoteAddress,
+ topicName, producerId, producerName, ex.getCause().getMessage());
+ commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
Review Comment:
I see that we're sending this message without checking the client's protocol version. I know that your PR didn't introduce this behavior, but I think we should make sure to handle that before this feature is released. Are you able to fix this @vineeth1995, or should we open an issue for the work?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1540,6 +1540,7 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
getPrincipal(), isEncrypted, metadata, schemaVersion, epoch,
userProvidedProducerName, producerAccessMode, topicEpoch, supportsPartialProducer);
+ log.info("trying to add producer - {} to topic -{}", producerName, topicName);
Review Comment:
We already log about this, so I think we can remove this before merging.
https://github.com/apache/pulsar/blob/8cc979de411f9bca05ea95b16807f1860b79382e/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L1374-L1377
--
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] vineeth1995 commented on pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "vineeth1995 (via GitHub)" <gi...@apache.org>.
vineeth1995 commented on PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#issuecomment-1502039907
/pulsarbot run-failure-checks
--
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] rdhabalia commented on a diff in pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "rdhabalia (via GitHub)" <gi...@apache.org>.
rdhabalia commented on code in PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#discussion_r1144077244
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1574,13 +1575,19 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
if (ex.getCause() instanceof BrokerServiceException.TopicMigratedException) {
Optional<ClusterUrl> clusterURL = getMigratedClusterUrl(service.getPulsar());
if (clusterURL.isPresent()) {
- log.info("[{}] redirect migrated producer to topic {}: producerId={}, {}", remoteAddress, topicName,
- producerId, ex.getCause().getMessage());
- commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
- clusterURL.get().getBrokerServiceUrl(), clusterURL.get().getBrokerServiceUrlTls());
- closeProducer(producer);
- return null;
-
+ if (topic.isReplicationBacklogExist()) {
+ log.info("Topic {} is migrated but replication backlog exist: "
+ + "producerId = {}, producerName = {}, {}", topicName,
+ producerId, producerName, ex.getCause().getMessage());
+ } else {
+ log.info("[{}] redirect migrated producer to topic {}: "
+ + "producerId={}, producerName = {}, {}", remoteAddress,
+ topicName, producerId, producerName, ex.getCause().getMessage());
+ commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
Review Comment:
we can also add broker-side feature to restrict specific client versions to activate this functionality as such a feature requires all clients to upgrade the version. but that can be addressed into a separate PR.
--
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] vineeth1995 closed pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "vineeth1995 (via GitHub)" <gi...@apache.org>.
vineeth1995 closed pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
URL: https://github.com/apache/pulsar/pull/19605
--
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] vineeth1995 commented on pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "vineeth1995 (via GitHub)" <gi...@apache.org>.
vineeth1995 commented on PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#issuecomment-1440616458
/pulsarbot run-failure-checks
--
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] rdhabalia commented on a diff in pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "rdhabalia (via GitHub)" <gi...@apache.org>.
rdhabalia commented on code in PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#discussion_r1120959144
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1574,13 +1575,19 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
if (ex.getCause() instanceof BrokerServiceException.TopicMigratedException) {
Optional<ClusterUrl> clusterURL = getMigratedClusterUrl(service.getPulsar());
if (clusterURL.isPresent()) {
- log.info("[{}] redirect migrated producer to topic {}: producerId={}, {}", remoteAddress, topicName,
- producerId, ex.getCause().getMessage());
- commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
- clusterURL.get().getBrokerServiceUrl(), clusterURL.get().getBrokerServiceUrlTls());
- closeProducer(producer);
- return null;
-
+ if (topic.isReplicationBacklogExist()) {
+ log.info("Topic {} is migrated but replication backlog exist: "
+ + "producerId = {}, producerName = {}, {}", topicName,
+ producerId, producerName, ex.getCause().getMessage());
+ } else {
+ log.info("[{}] redirect migrated producer to topic {}: "
+ + "producerId={}, producerName = {}, {}", remoteAddress,
+ topicName, producerId, producerName, ex.getCause().getMessage());
+ commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
Review Comment:
it's not exactly non-retriable error but client has to retry until topic backlog will be drained , after that producers will be redirected to a new cluster. it's exactly similar to backlog-quota exceeded error where it's non-retriable until backlog is drained.
--
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] dlg99 commented on a diff in pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "dlg99 (via GitHub)" <gi...@apache.org>.
dlg99 commented on code in PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#discussion_r1162016240
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1578,13 +1578,23 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
if (ex.getCause() instanceof BrokerServiceException.TopicMigratedException) {
Optional<ClusterUrl> clusterURL = getMigratedClusterUrl(service.getPulsar());
if (clusterURL.isPresent()) {
- log.info("[{}] redirect migrated producer to topic {}: producerId={}, {}", remoteAddress, topicName,
- producerId, ex.getCause().getMessage());
- commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
- clusterURL.get().getBrokerServiceUrl(), clusterURL.get().getBrokerServiceUrlTls());
- closeProducer(producer);
- return null;
-
+ if (topic.isReplicationBacklogExist()) {
+ log.info("Topic {} is migrated but replication backlog exist: "
Review Comment:
Is it possible for isReplicationBacklogExist to become true later?
e.g. on one pass all replicators don't have backlog (isReplicationBacklogExist == false), next time one of the replicators gets more messages/backlog?
What guards against this case / is it a problem?
--
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] vineeth1995 commented on pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "vineeth1995 (via GitHub)" <gi...@apache.org>.
vineeth1995 commented on PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#issuecomment-1464252845
/pulsarbot run-failure-checks
--
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] rdhabalia merged pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "rdhabalia (via GitHub)" <gi...@apache.org>.
rdhabalia merged PR #19605:
URL: https://github.com/apache/pulsar/pull/19605
--
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] lhotari commented on pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#issuecomment-1559562028
There seems to be a new flaky test introduced by this PR. The flaky test issue is #20375. @vineeth1995 do you have a chance to take a look? thanks
--
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] vineeth1995 commented on pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "vineeth1995 (via GitHub)" <gi...@apache.org>.
vineeth1995 commented on PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#issuecomment-1559664453
Hi,
I will create a fix for this.
Thanks,
Vineeth
On Tue, May 23, 2023 at 7:40 AM Lari Hotari ***@***.***>
wrote:
> There seems to be a new flaky test introduced by this PR. The flaky test
> issue is #20375 <https://github.com/apache/pulsar/issues/20375>.
> @vineeth1995 <https://github.com/vineeth1995> do you have a chance to
> take a look? thanks
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/pulsar/pull/19605#issuecomment-1559562028>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGAMY76GHMT6LJ2II2YTX3DXHTD7JANCNFSM6AAAAAAVEVRY6M>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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] vineeth1995 commented on a diff in pull request #19605: [feat] [broker] PIP-188 support blue-green cluster migration [part-2]
Posted by "vineeth1995 (via GitHub)" <gi...@apache.org>.
vineeth1995 commented on code in PR #19605:
URL: https://github.com/apache/pulsar/pull/19605#discussion_r1129801986
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1574,13 +1575,19 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
if (ex.getCause() instanceof BrokerServiceException.TopicMigratedException) {
Optional<ClusterUrl> clusterURL = getMigratedClusterUrl(service.getPulsar());
if (clusterURL.isPresent()) {
- log.info("[{}] redirect migrated producer to topic {}: producerId={}, {}", remoteAddress, topicName,
- producerId, ex.getCause().getMessage());
- commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
- clusterURL.get().getBrokerServiceUrl(), clusterURL.get().getBrokerServiceUrlTls());
- closeProducer(producer);
- return null;
-
+ if (topic.isReplicationBacklogExist()) {
+ log.info("Topic {} is migrated but replication backlog exist: "
+ + "producerId = {}, producerName = {}, {}", topicName,
+ producerId, producerName, ex.getCause().getMessage());
+ } else {
+ log.info("[{}] redirect migrated producer to topic {}: "
+ + "producerId={}, producerName = {}, {}", remoteAddress,
+ topicName, producerId, producerName, ex.getCause().getMessage());
+ commandSender.sendTopicMigrated(ResourceType.Producer, producerId,
Review Comment:
@michaeljmarshall will handle this in a separate pr as this requires client side change as well.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1540,6 +1540,7 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
getPrincipal(), isEncrypted, metadata, schemaVersion, epoch,
userProvidedProducerName, producerAccessMode, topicEpoch, supportsPartialProducer);
+ log.info("trying to add producer - {} to topic -{}", producerName, topicName);
Review Comment:
addressed
--
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