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/08 12:54:09 UTC
[GitHub] [pulsar] Omega-Ariston opened a new pull request #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Omega-Ariston opened a new pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920
Fixes #6887
### Motivation
The ttl for namespaces should be retrieved from broker's configuration if it is not configured at namespace policies. However, the current code only returns the value stored in namespace policies directly without judging if it is configured or not.
### Modifications
Added a condition to test if ttl is configured at namespace policies. If not, retrieve value stored in broker's configuration and return it as output.
----------------------------------------------------------------
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] trexinc commented on pull request #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
trexinc commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-625922882
As I see it - it is actually a nice feature. You can changes the global default TTL and it immediately applies to all namespaces with default 0 TTL.
So probably just best to document it, and not make any code changes.
----------------------------------------------------------------
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] trexinc commented on pull request #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
trexinc commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-625903891
In the broker in
----------------------------------------------------------------
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 #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-626086632
@Omega-Ariston I think you can fix the `mergeNamespaceWithDefaults` method in the AdminResource class, we merge all namespace policy with the broker configuration here.
----------------------------------------------------------------
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] Omega-Ariston removed a comment on pull request #6920: [Issue 6887][pulsar-broker] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
Omega-Ariston removed a comment on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-625841066
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] trexinc commented on pull request #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
trexinc commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-625883199
I don't think this is a valid fix. It fixes only the cli, creating a namespace using API will still have the issue.
----------------------------------------------------------------
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] trexinc removed a comment on pull request #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
trexinc removed a comment on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-625903891
In the broker in
----------------------------------------------------------------
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] Omega-Ariston commented on pull request #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
Omega-Ariston commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-625841066
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Omega-Ariston commented on pull request #6920: [Issue 6887][pulsar-broker] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
Omega-Ariston commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-626113627
> @Omega-Ariston I think you can fix the `mergeNamespaceWithDefaults` method in the AdminResource class, we merge all namespace policy with the broker configuration here. And please add some unit tests for the change to avoid some other changes to break it.
Okay thx, working on it :)
----------------------------------------------------------------
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 edited a comment on pull request #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
codelipenghui edited a comment on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-626086632
@Omega-Ariston I think you can fix the `mergeNamespaceWithDefaults` method in the AdminResource class, we merge all namespace policy with the broker configuration here. And please add some unit tests for the change to avoid some other changes to break it.
----------------------------------------------------------------
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] jiazhai commented on pull request #6920: [Issue 6887][pulsar-broker] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-626409095
@Omega-Ariston as Penghui mentioned, would you please help add a unit tests for 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] trexinc commented on pull request #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
trexinc commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-625925723
@Omega-Ariston, sorry, had a long day and didn't even notice that your fix is for the "cosmetic" issue.
One issue with the fix that I see is that after it you can't tell which namespaces have the default 0 TTL. So maybe the command should print both global default and the one set on the namespace.
----------------------------------------------------------------
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] trexinc commented on pull request #6920: [Issue 6887][pulsar-client-tools] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
trexinc commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-625905818
@codelipenghui @Omega-Ariston - there is actually the following code in the broker. So it seems that it does take the default TTL into consideration at the actual expiry check. So the bug is kind of more cosmetic than anything else.
Do you think that maybe get-namespace-ttl API should return the effective TTL?
@Override
public void checkMessageExpiry() {
TopicName name = TopicName.get(topic);
Policies policies;
try {
policies = brokerService.pulsar().getConfigurationCache().policiesCache()
.get(AdminResource.path(POLICIES, name.getNamespace()))
.orElseThrow(() -> new KeeperException.NoNodeException());
int defaultTTL = brokerService.pulsar().getConfiguration().getTtlDurationDefaultInSeconds();
int message_ttl_in_seconds = (policies.message_ttl_in_seconds <= 0 && defaultTTL > 0) ? defaultTTL
: policies.message_ttl_in_seconds;
if (message_ttl_in_seconds != 0) {
subscriptions.forEach((subName, sub) -> sub.expireMessages(message_ttl_in_seconds));
replicators.forEach((region, replicator) -> ((PersistentReplicator)replicator).expireMessages(message_ttl_in_seconds));
}
} catch (Exception e) {
if (log.isDebugEnabled()) {
log.debug("[{}] Error getting policies", topic);
}
}
}
----------------------------------------------------------------
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] Omega-Ariston commented on pull request #6920: [Issue 6887][pulsar-broker] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
Omega-Ariston commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-626113762
> As I see it - it is actually a nice feature. You can changes the global default TTL and it immediately applies to all namespaces with default 0 TTL.
> So probably just best to document it, and not make any code changes.
Maybe returning the actual working TTL value is what we want here, as you said it's more like a cosmetic fix :D
----------------------------------------------------------------
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] Omega-Ariston commented on pull request #6920: [Issue 6887][pulsar-broker] ttlDurationDefaultInSeconds not applying
Posted by GitBox <gi...@apache.org>.
Omega-Ariston commented on pull request #6920:
URL: https://github.com/apache/pulsar/pull/6920#issuecomment-626100992
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org