You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/05/21 14:11:19 UTC
[GitHub] [pulsar] codelipenghui opened a new pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
codelipenghui opened a new pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011
### Motivation
Fix break changes in namespace offload policy and RabbitMQ sink.
### 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: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
- Does this pull request introduce a new feature? (no)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#issuecomment-632616152
@merlimat @srkukarni Tests is passed, I merged this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#discussion_r429099176
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -539,6 +544,25 @@ public Boolean get() {
}
}
+ private void generatePropertiesForConfigurationIfNeeded() {
Review comment:
Since the purpose it's not obvious, can you add a comment on why we need this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] srkukarni commented on pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
srkukarni commented on pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#issuecomment-632460493
The most recent failure was in replicator test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] srkukarni commented on pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
srkukarni commented on pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#issuecomment-632444012
The broker unit tests have failed a lot of times. Maybe something to do with this change?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#discussion_r428760906
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -1062,17 +1062,6 @@ public void openLedgerFailed(ManagedLedgerException exception, Object ctx) {
managedLedgerConfig.setRetentionSizeInMB(retentionPolicies.getRetentionSizeInMB());
managedLedgerConfig.setAutoSkipNonRecoverableData(serviceConfig.isAutoSkipNonRecoverableData());
OffloadPolicies offloadPolicies = policies.map(p -> p.offload_policies).orElse(null);
-
- if (offloadPolicies == null) {
Review comment:
This code was there to fallback to defaults in case there is no special offload policy for a given namespace. How is this behavior maintained?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#discussion_r428788947
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -1062,17 +1062,6 @@ public void openLedgerFailed(ManagedLedgerException exception, Object ctx) {
managedLedgerConfig.setRetentionSizeInMB(retentionPolicies.getRetentionSizeInMB());
managedLedgerConfig.setAutoSkipNonRecoverableData(serviceConfig.isAutoSkipNonRecoverableData());
OffloadPolicies offloadPolicies = policies.map(p -> p.offload_policies).orElse(null);
-
- if (offloadPolicies == null) {
Review comment:
It's handled by https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java#L813
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#discussion_r428840205
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -1062,17 +1062,6 @@ public void openLedgerFailed(ManagedLedgerException exception, Object ctx) {
managedLedgerConfig.setRetentionSizeInMB(retentionPolicies.getRetentionSizeInMB());
managedLedgerConfig.setAutoSkipNonRecoverableData(serviceConfig.isAutoSkipNonRecoverableData());
OffloadPolicies offloadPolicies = policies.map(p -> p.offload_policies).orElse(null);
-
- if (offloadPolicies == null) {
Review comment:
Got it. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] srkukarni commented on pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
srkukarni commented on pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#issuecomment-632492538
@codelipenghui another error in filesystemoffload integration test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#discussion_r429109786
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -539,6 +544,25 @@ public Boolean get() {
}
}
+ private void generatePropertiesForConfigurationIfNeeded() {
Review comment:
Removed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#issuecomment-632448020
@srkukarni How can I get the previous error log? I think we can paste the failed tests under the comments. So that I can reproduce it on my laptop.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] skyrocknroll commented on pull request #7011: Fix break changes in namespace offload policy and RabbitMQ sink.
Posted by GitBox <gi...@apache.org>.
skyrocknroll commented on pull request #7011:
URL: https://github.com/apache/pulsar/pull/7011#issuecomment-635753379
@sijie @codelipenghui Should we include this as part of 2.5.3 release? Considering that this bug is causing offloading to fail?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org