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 2021/09/02 07:36:08 UTC
[GitHub] [pulsar] eolivelli opened a new pull request #11894: [Issue 11814] fix pulsar admin method:getMessageById. (#11852)
eolivelli opened a new pull request #11894:
URL: https://github.com/apache/pulsar/pull/11894
Fix https://github.com/apache/pulsar/issues/11814 , if we use another topic to find the message, it will return the message, but we may contaminate the ledgers cache in the topic.
**changes**
Add check in the method 'internalGetMessageById' in PersistentTopicsBase, if the ledgerId not belong to this topic, throw a exception.
(cherry picked from commit 9bfb3dba7c13e8250e1002efdeb39eec56f7e2da)
--
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] merlimat commented on a change in pull request #11894: [Issue 11814] fix pulsar admin method:getMessageById. (#11852)
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11894:
URL: https://github.com/apache/pulsar/pull/11894#discussion_r701509896
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2170,6 +2170,14 @@ protected void internalGetMessageById(AsyncResponse asyncResponse, long ledgerId
validateReadOperationOnTopic(authoritative);
PersistentTopic topic = (PersistentTopic) getTopicReference(topicName);
ManagedLedgerImpl ledger = (ManagedLedgerImpl) topic.getManagedLedger();
+ if (null == ledger.getLedgerInfo(ledgerId).get()) {
Review comment:
Agree, this should be done asynchronously
--
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] eolivelli commented on pull request #11894: [Issue 11814] fix pulsar admin method:getMessageById. (#11852)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11894:
URL: https://github.com/apache/pulsar/pull/11894#issuecomment-911320607
@zhanghaou I have ported your fix to branch-2.7, please take a look
--
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 a change in pull request #11894: [Issue 11814] fix pulsar admin method:getMessageById. (#11852)
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11894:
URL: https://github.com/apache/pulsar/pull/11894#discussion_r701148343
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2170,6 +2170,14 @@ protected void internalGetMessageById(AsyncResponse asyncResponse, long ledgerId
validateReadOperationOnTopic(authoritative);
PersistentTopic topic = (PersistentTopic) getTopicReference(topicName);
ManagedLedgerImpl ledger = (ManagedLedgerImpl) topic.getManagedLedger();
+ if (null == ledger.getLedgerInfo(ledgerId).get()) {
Review comment:
Is it fine to perform a blocking method call 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zhanghaou commented on a change in pull request #11894: [Issue 11814] fix pulsar admin method:getMessageById. (#11852)
Posted by GitBox <gi...@apache.org>.
zhanghaou commented on a change in pull request #11894:
URL: https://github.com/apache/pulsar/pull/11894#discussion_r701807659
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2170,6 +2170,14 @@ protected void internalGetMessageById(AsyncResponse asyncResponse, long ledgerId
validateReadOperationOnTopic(authoritative);
PersistentTopic topic = (PersistentTopic) getTopicReference(topicName);
ManagedLedgerImpl ledger = (ManagedLedgerImpl) topic.getManagedLedger();
+ if (null == ledger.getLedgerInfo(ledgerId).get()) {
Review comment:
It seems it is not run in a netty thread. btw, https://github.com/apache/pulsar/pull/11912 is better to fix this bug.
--
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] eolivelli closed pull request #11894: [Issue 11814] fix pulsar admin method:getMessageById. (#11852)
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #11894:
URL: https://github.com/apache/pulsar/pull/11894
--
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] zhanghaou commented on a change in pull request #11894: [Issue 11814] fix pulsar admin method:getMessageById. (#11852)
Posted by GitBox <gi...@apache.org>.
zhanghaou commented on a change in pull request #11894:
URL: https://github.com/apache/pulsar/pull/11894#discussion_r701567534
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2170,6 +2170,14 @@ protected void internalGetMessageById(AsyncResponse asyncResponse, long ledgerId
validateReadOperationOnTopic(authoritative);
PersistentTopic topic = (PersistentTopic) getTopicReference(topicName);
ManagedLedgerImpl ledger = (ManagedLedgerImpl) topic.getManagedLedger();
+ if (null == ledger.getLedgerInfo(ledgerId).get()) {
Review comment:
It seems that theres no time-consuming operation, any other point needs to consider?
--
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 a change in pull request #11894: [Issue 11814] fix pulsar admin method:getMessageById. (#11852)
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11894:
URL: https://github.com/apache/pulsar/pull/11894#discussion_r701749010
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2170,6 +2170,14 @@ protected void internalGetMessageById(AsyncResponse asyncResponse, long ledgerId
validateReadOperationOnTopic(authoritative);
PersistentTopic topic = (PersistentTopic) getTopicReference(topicName);
ManagedLedgerImpl ledger = (ManagedLedgerImpl) topic.getManagedLedger();
+ if (null == ledger.getLedgerInfo(ledgerId).get()) {
Review comment:
It's not about time. A Netty threads should never be blocked.
--
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] Anonymitaet commented on pull request #11894: [Issue 11814] fix pulsar admin method:getMessageById. (#11852)
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11894:
URL: https://github.com/apache/pulsar/pull/11894#issuecomment-912190014
Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? 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