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/02/24 16:19:13 UTC
[GitHub] [pulsar] RobertIndie opened a new pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
RobertIndie opened a new pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460
### Motivation
There are some unused codes about entry metadata. It's better to remove them.
### Modifications
* Remove some unused codes about `AddEntryMetadataException` and `addBrokerEntryMetadata`
### Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
### Documentation
- [x] `no-need-doc`
--
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] michaeljmarshall commented on a change in pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#discussion_r814960550
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java
##########
@@ -1525,11 +1525,6 @@ private static ByteBufPair serializeCommandSendWithSize(BaseCommand cmd, Checksu
return command;
}
- public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
Review comment:
This could be used by 3rd party clients. It is part of our public API.
--
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] codelipenghui commented on a change in pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#discussion_r815386247
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java
##########
@@ -1525,11 +1525,6 @@ private static ByteBufPair serializeCommandSendWithSize(BaseCommand cmd, Checksu
return command;
}
- public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
Review comment:
@eolivelli Do you know which protocol handler uses this method? I'm curious how the protocol handler works with the method, add broker entry metadata is only used by the managed ledger internal. If the protocol handler wants to introduce broker entry metadata changes, it should implement a new `BrokerEntryMetadataInterceptor`.
IMO, essentially, this is not a public API at the beginning of the design, It's just that all the pulsar command-related methods are put here.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerServiceException.java
##########
@@ -77,12 +77,6 @@ public TopicClosedException(Throwable t) {
}
}
- public static class AddEntryMetadataException extends BrokerServiceException {
Review comment:
@michaeljmarshall For the protocol handler, it should not touch this exception. Here is an example https://github.com/apache/pulsar/pull/9039/files#diff-e7b41a1d0ffec3009ff825684ca865095884454d29d81b8f0edf6a6c129e21f4R26-R53 for adding a new entry metadata interceptor, I don't think a protocol handler will touch this exception unless the protocol handler implements it in the wrong way. This is the part of the implementation that was forgotten to delete at the time (in 2.8.0). I have no objection with deprecating it first, just feel like we're adding complexity to something that was should be removed and never used.
--
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] codelipenghui commented on pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#issuecomment-1067597319
@RobertIndie Could you please make an update to `deprecated` first?
--
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] RobertIndie commented on pull request #14460: [refactor][broker] Deprecate `AddEntryMetadataException`
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#issuecomment-1073644084
@codelipenghui @michaeljmarshall @eolivelli I have changed this PR to deprecate the `AddEntryMetadataException`. PTAL. 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
[GitHub] [pulsar] RobertIndie commented on pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#issuecomment-1050416027
/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] eolivelli commented on a change in pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#discussion_r814964154
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java
##########
@@ -1525,11 +1525,6 @@ private static ByteBufPair serializeCommandSendWithSize(BaseCommand cmd, Checksu
return command;
}
- public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
Review comment:
Unfortunately we have protocol handlers and we can't easily drop 'public' classes and methods
--
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] michaeljmarshall commented on a change in pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#discussion_r814492037
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerServiceException.java
##########
@@ -77,12 +77,6 @@ public TopicClosedException(Throwable t) {
}
}
- public static class AddEntryMetadataException extends BrokerServiceException {
Review comment:
I see that this isn't removed, we should deprecate the class.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java
##########
@@ -1525,11 +1525,6 @@ private static ByteBufPair serializeCommandSendWithSize(BaseCommand cmd, Checksu
return command;
}
- public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
Review comment:
What is the reason for removing this one?
--
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] RobertIndie commented on a change in pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#discussion_r814654964
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerServiceException.java
##########
@@ -77,12 +77,6 @@ public TopicClosedException(Throwable t) {
}
}
- public static class AddEntryMetadataException extends BrokerServiceException {
Review comment:
There is no other code that uses this exception.
--
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] RobertIndie commented on pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#issuecomment-1050440730
/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] michaeljmarshall commented on a change in pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#discussion_r814962778
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerServiceException.java
##########
@@ -77,12 +77,6 @@ public TopicClosedException(Throwable t) {
}
}
- public static class AddEntryMetadataException extends BrokerServiceException {
Review comment:
This might not be thrown by Pulsar, but a Pulsar Protocol handler could try to catch it. Therefore, I think we need to mark it as deprecated before we remove 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#issuecomment-1051055206
@codelipenghui - while the method/class might not be used by Pulsar internals, they are publicly accessible to clients/protocol handlers, so I think we need to take an incremental approach. We could mark as deprecated for 2.11 and then remove them in 2.12, for example.
--
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] RobertIndie commented on a change in pull request #14460: [Broker] Remove `AddEntryMetadataException` and `addBrokerEntryMetadata`
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460#discussion_r814654710
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java
##########
@@ -1525,11 +1525,6 @@ private static ByteBufPair serializeCommandSendWithSize(BaseCommand cmd, Checksu
return command;
}
- public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
Review comment:
There is no other code that invokes this method.
--
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 merged pull request #14460: [refactor][broker] Deprecate `AddEntryMetadataException`
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #14460:
URL: https://github.com/apache/pulsar/pull/14460
--
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