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 2022/12/02 06:47:07 UTC
[GitHub] [pulsar] labuladong opened a new pull request, #18709: [improve][broker] improve shadow topic error message
labuladong opened a new pull request, #18709:
URL: https://github.com/apache/pulsar/pull/18709
### Motivation
[PIP-180](https://github.com/apache/pulsar/issues/16153) add shadow topic feature. Shadow topic is read-only, but if I send msg to a shadow topic, the error message is `"Only shadow topic supports sending messages with messageId"`, which is confusing.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
### 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 -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
PTAL @Jason918
--
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] gaozhangmin merged pull request #18709: [improve][broker] improve shadow topic error message
Posted by GitBox <gi...@apache.org>.
gaozhangmin merged PR #18709:
URL: https://github.com/apache/pulsar/pull/18709
--
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] labuladong commented on pull request #18709: [improve][broker] improve shadow topic error message
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18709:
URL: https://github.com/apache/pulsar/pull/18709#issuecomment-1336360052
/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] gaozhangmin commented on a diff in pull request #18709: [improve][broker] improve shadow topic error message
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #18709:
URL: https://github.com/apache/pulsar/pull/18709#discussion_r1037841242
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java:
##########
@@ -210,14 +210,22 @@ public void publishMessage(long producerId, long lowestSequenceId, long highestS
public boolean checkAndStartPublish(long producerId, long sequenceId, ByteBuf headersAndPayload, long batchSize,
Position position) {
- if (isShadowTopic && position == null || !isShadowTopic && position != null) {
+ if (!isShadowTopic && position != null) {
cnx.execute(() -> {
cnx.getCommandSender().sendSendError(producerId, sequenceId, ServerError.NotAllowedError,
"Only shadow topic supports sending messages with messageId");
cnx.completedSendOperation(isNonPersistentTopic, headersAndPayload.readableBytes());
});
return false;
}
+ if (isShadowTopic && position == null) {
+ cnx.execute(() -> {
+ cnx.getCommandSender().sendSendError(producerId, sequenceId, ServerError.NotAllowedError,
+ "Cannot send messages to a shadow topic");
Review Comment:
We'd better tell this error is caused by `position == null`
--
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] labuladong commented on a diff in pull request #18709: [improve][broker] improve shadow topic error message
Posted by GitBox <gi...@apache.org>.
labuladong commented on code in PR #18709:
URL: https://github.com/apache/pulsar/pull/18709#discussion_r1037857479
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java:
##########
@@ -210,14 +210,22 @@ public void publishMessage(long producerId, long lowestSequenceId, long highestS
public boolean checkAndStartPublish(long producerId, long sequenceId, ByteBuf headersAndPayload, long batchSize,
Position position) {
- if (isShadowTopic && position == null || !isShadowTopic && position != null) {
+ if (!isShadowTopic && position != null) {
cnx.execute(() -> {
cnx.getCommandSender().sendSendError(producerId, sequenceId, ServerError.NotAllowedError,
"Only shadow topic supports sending messages with messageId");
cnx.completedSendOperation(isNonPersistentTopic, headersAndPayload.readableBytes());
});
return false;
}
+ if (isShadowTopic && position == null) {
+ cnx.execute(() -> {
+ cnx.getCommandSender().sendSendError(producerId, sequenceId, ServerError.NotAllowedError,
+ "Cannot send messages to a shadow topic");
Review Comment:
This `position` is only used in shadow topic internal replication:
https://github.com/apache/pulsar/blob/5b062f37bd6688c09384abbe908f7c14e28052fc/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L1598-L1602
`position` should be `null` for regular producers. So I think it's not necessary to tell users about this detail.
--
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] labuladong commented on pull request #18709: [improve][broker] improve shadow topic error message
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18709:
URL: https://github.com/apache/pulsar/pull/18709#issuecomment-1336748599
/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] Jason918 commented on pull request #18709: [improve][broker] improve shadow topic error message
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #18709:
URL: https://github.com/apache/pulsar/pull/18709#issuecomment-1336345438
/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] codecov-commenter commented on pull request #18709: [improve][broker] improve shadow topic error message
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18709:
URL: https://github.com/apache/pulsar/pull/18709#issuecomment-1336347922
# [Codecov](https://codecov.io/gh/apache/pulsar/pull/18709?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18709](https://codecov.io/gh/apache/pulsar/pull/18709?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91c08fc) into [master](https://codecov.io/gh/apache/pulsar/commit/90c5534cd75275613a014640ea6283afdf1721d3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (90c5534) will **decrease** coverage by `12.97%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/pulsar/pull/18709?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18709 +/- ##
=============================================
- Coverage 50.05% 37.08% -12.98%
+ Complexity 9706 1968 -7738
=============================================
Files 618 209 -409
Lines 58586 14421 -44165
Branches 6098 1573 -4525
=============================================
- Hits 29327 5348 -23979
+ Misses 26092 8487 -17605
+ Partials 3167 586 -2581
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `37.08% <ø> (-12.98%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18709?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...g/apache/pulsar/client/impl/ConnectionHandler.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0Nvbm5lY3Rpb25IYW5kbGVyLmphdmE=) | `50.00% <0.00%> (-5.32%)` | :arrow_down: |
| [.../org/apache/pulsar/client/impl/ConnectionPool.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0Nvbm5lY3Rpb25Qb29sLmphdmE=) | `37.43% <0.00%> (-1.03%)` | :arrow_down: |
| [...ava/org/apache/pulsar/broker/service/Producer.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1Byb2R1Y2VyLmphdmE=) | | |
| [.../main/java/org/apache/pulsar/PulsarStandalone.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL1B1bHNhclN0YW5kYWxvbmUuamF2YQ==) | | |
| [...ransaction/buffer/impl/InMemTransactionBuffer.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci90cmFuc2FjdGlvbi9idWZmZXIvaW1wbC9Jbk1lbVRyYW5zYWN0aW9uQnVmZmVyLmphdmE=) | | |
| [...a/org/apache/pulsar/broker/web/RequestWrapper.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci93ZWIvUmVxdWVzdFdyYXBwZXIuamF2YQ==) | | |
| [...pulsar/proxy/server/ServiceChannelInitializer.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLXByb3h5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvcHJveHkvc2VydmVyL1NlcnZpY2VDaGFubmVsSW5pdGlhbGl6ZXIuamF2YQ==) | | |
| [.../transaction/pendingack/PendingAckHandleStats.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci90cmFuc2FjdGlvbi9wZW5kaW5nYWNrL1BlbmRpbmdBY2tIYW5kbGVTdGF0cy5qYXZh) | | |
| [...ache/pulsar/proxy/server/BrokerProxyValidator.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLXByb3h5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvcHJveHkvc2VydmVyL0Jyb2tlclByb3h5VmFsaWRhdG9yLmphdmE=) | | |
| [...g/apache/pulsar/PulsarClusterMetadataTeardown.java](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL1B1bHNhckNsdXN0ZXJNZXRhZGF0YVRlYXJkb3duLmphdmE=) | | |
| ... and [401 more](https://codecov.io/gh/apache/pulsar/pull/18709/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
--
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] Jason918 commented on a diff in pull request #18709: [improve][broker] improve shadow topic error message
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #18709:
URL: https://github.com/apache/pulsar/pull/18709#discussion_r1038926048
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java:
##########
@@ -210,14 +210,22 @@ public void publishMessage(long producerId, long lowestSequenceId, long highestS
public boolean checkAndStartPublish(long producerId, long sequenceId, ByteBuf headersAndPayload, long batchSize,
Position position) {
- if (isShadowTopic && position == null || !isShadowTopic && position != null) {
+ if (!isShadowTopic && position != null) {
cnx.execute(() -> {
cnx.getCommandSender().sendSendError(producerId, sequenceId, ServerError.NotAllowedError,
"Only shadow topic supports sending messages with messageId");
cnx.completedSendOperation(isNonPersistentTopic, headersAndPayload.readableBytes());
});
return false;
}
+ if (isShadowTopic && position == null) {
+ cnx.execute(() -> {
+ cnx.getCommandSender().sendSendError(producerId, sequenceId, ServerError.NotAllowedError,
+ "Cannot send messages to a shadow topic");
Review Comment:
> `position` should be `null` for regular producers.
Correct. It's only used in `ShadowReplicator`.
--
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] labuladong commented on pull request #18709: [improve][broker] improve shadow topic error message
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18709:
URL: https://github.com/apache/pulsar/pull/18709#issuecomment-1336961759
This pr is ready to merge ^_^
--
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