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/10/02 01:12:28 UTC
[GitHub] [pulsar] hangc0276 opened a new pull request #8181: fix peek message not supported for partitioned topic with topicname
hangc0276 opened a new pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181
Fix #8174
### Motivation
To Reproduce
Steps to reproduce the behavior:
```shell
bin/pulsar-admin topics create-partitioned-topic persistent://public/default/test-v1
pulsar-admin topics create-subscription -s sub persistent://public/default/test-v1
pulsar-admin topics peek-messages -s sub persitent://public/default/test-v1
```
Throw exception:
```
Peek messages on a partitioned topic is not allowed
```
The reseason is in PersistentTopicsBase.java#internalPeekNthMessage method, it check the partition number for the given topicName. The `persitent://public/default/test-v1` is not partitioned and the partition size > 0 in the following check, so throw `METHOD_NOT_ALLOWED` exception.
```Java
if (!topicName.isPartitioned() && getPartitionedTopicMetadata(topicName, authoritative, false).partitions > 0) {
throw new RestException(Status.METHOD_NOT_ALLOWED, "Peek messages on a partitioned topic is not allowed");
}
```
### Changes
Using the partition name instead of topicName in this case.
Please take a look @sijie @codelipenghui @tuteng
----------------------------------------------------------------
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] hangc0276 commented on a change in pull request #8181: fix peek message not supported for partitioned topic with topicname
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181#discussion_r499083633
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2089,20 +2089,23 @@ public void readEntryComplete(Entry entry, Object ctx) {
}
protected Response internalPeekNthMessage(String subName, int messagePosition, boolean authoritative) {
- verifyReadOperation(authoritative);
+ verifyReadOperation();
+
+ TopicName partitionedTopicName = topicName;
// If the topic name is a partition name, no need to get partition topic metadata again
if (!topicName.isPartitioned() && getPartitionedTopicMetadata(topicName, authoritative, false).partitions > 0) {
- throw new RestException(Status.METHOD_NOT_ALLOWED, "Peek messages on a partitioned topic is not allowed");
+ partitionedTopicName = topicName.getPartition(0);
Review comment:
@sijie Yes, I Can't reproduce the case of #8174 described by @tuteng . Maybe the Pulsar manager peeking message is actually called Pulsar admin API described by me.
----------------------------------------------------------------
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] sijie commented on a change in pull request #8181: fix peek message not supported for partitioned topic with topicname
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181#discussion_r498897819
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2089,20 +2089,23 @@ public void readEntryComplete(Entry entry, Object ctx) {
}
protected Response internalPeekNthMessage(String subName, int messagePosition, boolean authoritative) {
- verifyReadOperation(authoritative);
+ verifyReadOperation();
+
+ TopicName partitionedTopicName = topicName;
// If the topic name is a partition name, no need to get partition topic metadata again
if (!topicName.isPartitioned() && getPartitionedTopicMetadata(topicName, authoritative, false).partitions > 0) {
- throw new RestException(Status.METHOD_NOT_ALLOWED, "Peek messages on a partitioned topic is not allowed");
+ partitionedTopicName = topicName.getPartition(0);
Review comment:
@hangc0276 Actually I am a bit confused with #8174. Need some clarification from @tuteng. I thought Pulsar manager doesn't work when peeking messages from a partition of a partitioned topic because Pulsar admin API doesn't support that. That was the reason @tuteng created #8174 . @tuteng Can you confirm?
----------------------------------------------------------------
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] hangc0276 commented on a change in pull request #8181: fix peek message not supported for partitioned topic with topicname
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181#discussion_r498596461
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2089,20 +2089,23 @@ public void readEntryComplete(Entry entry, Object ctx) {
}
protected Response internalPeekNthMessage(String subName, int messagePosition, boolean authoritative) {
- verifyReadOperation(authoritative);
+ verifyReadOperation();
+
+ TopicName partitionedTopicName = topicName;
// If the topic name is a partition name, no need to get partition topic metadata again
if (!topicName.isPartitioned() && getPartitionedTopicMetadata(topicName, authoritative, false).partitions > 0) {
- throw new RestException(Status.METHOD_NOT_ALLOWED, "Peek messages on a partitioned topic is not allowed");
+ partitionedTopicName = topicName.getPartition(0);
Review comment:
Yes, if we support peeking messages on a partitioned topic, it may cause message inconsistent. I my current implementation, i just peek the partition 0 messages.
In this case, issue #8174 can be closed.
----------------------------------------------------------------
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] sijie commented on a change in pull request #8181: fix peek message not supported for partitioned topic with topicname
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181#discussion_r499087336
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2089,20 +2089,23 @@ public void readEntryComplete(Entry entry, Object ctx) {
}
protected Response internalPeekNthMessage(String subName, int messagePosition, boolean authoritative) {
- verifyReadOperation(authoritative);
+ verifyReadOperation();
+
+ TopicName partitionedTopicName = topicName;
// If the topic name is a partition name, no need to get partition topic metadata again
if (!topicName.isPartitioned() && getPartitionedTopicMetadata(topicName, authoritative, false).partitions > 0) {
- throw new RestException(Status.METHOD_NOT_ALLOWED, "Peek messages on a partitioned topic is not allowed");
+ partitionedTopicName = topicName.getPartition(0);
Review comment:
I see. If that's the case, we need to fix the problem in Pulsar Manager. Pulsar Manager should use the partition name to peek messages.
----------------------------------------------------------------
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 #8181: fix peek message not supported for partitioned topic with topicname
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181#issuecomment-703225431
Close this PR first, @hangc0276 you can reopen it if needed.
----------------------------------------------------------------
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] tuteng edited a comment on pull request #8181: fix peek message not supported for partitioned topic with topicname
Posted by GitBox <gi...@apache.org>.
tuteng edited a comment on pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181#issuecomment-703193646
Thanks for your contribution, I have reconfirmed that this command is OK. we can close this pr @sijie @hangc0276
```
pulsar-admin topics peek-messages -s sub topic-name-partitioned-0
```
----------------------------------------------------------------
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] tuteng commented on pull request #8181: fix peek message not supported for partitioned topic with topicname
Posted by GitBox <gi...@apache.org>.
tuteng commented on pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181#issuecomment-703193646
Thanks for your contribution, I have reconfirmed that this command is OK. we can close this or @sijie @hangc0276
```
pulsar-admin topics peek-messages -s sub topic-name-partitioned-0
```
----------------------------------------------------------------
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] sijie commented on a change in pull request #8181: fix peek message not supported for partitioned topic with topicname
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181#discussion_r498590013
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -2089,20 +2089,23 @@ public void readEntryComplete(Entry entry, Object ctx) {
}
protected Response internalPeekNthMessage(String subName, int messagePosition, boolean authoritative) {
- verifyReadOperation(authoritative);
+ verifyReadOperation();
+
+ TopicName partitionedTopicName = topicName;
// If the topic name is a partition name, no need to get partition topic metadata again
if (!topicName.isPartitioned() && getPartitionedTopicMetadata(topicName, authoritative, false).partitions > 0) {
- throw new RestException(Status.METHOD_NOT_ALLOWED, "Peek messages on a partitioned topic is not allowed");
+ partitionedTopicName = topicName.getPartition(0);
Review comment:
@hangc0276 I think it is true that we don't want to support peeking messages on a partitioned topic. But we want to support peeking messages on a partition of a partitioned topic.
E.g. if topic `test-topic` is a partitioned topic, it should fail if a user attempts to peek messages on topic `test-topic`. But it should succeed if a users attempts to peek messages from its partitions like `test-topic-partition-0`.
----------------------------------------------------------------
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 closed pull request #8181: fix peek message not supported for partitioned topic with topicname
Posted by GitBox <gi...@apache.org>.
codelipenghui closed pull request #8181:
URL: https://github.com/apache/pulsar/pull/8181
----------------------------------------------------------------
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