michaeljmarshall commented on code in PR #15147:
URL: https://github.com/apache/pulsar/pull/15147#discussion_r854772834
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/BrokerVersionFilter.java:
##########
@@ -106,17 +106,15 @@ public Version getLatestVersionNumber(Set<String>
brokers, LoadData loadData)
/**
* From the given set of available broker candidates, filter those using
the version numbers.
- *
- * @param brokers
+ * @param brokers
* The currently available brokers that have not already been
filtered.
* @param bundleToAssign
- * The data for the bundle to assign.
* @param loadData
* The load data from the leader broker.
* @param conf
- * The service configuration.
*/
- public void filter(Set<String> brokers, BundleData bundleToAssign,
LoadData loadData, ServiceConfiguration conf)
+ public void filter(Set<String> brokers, BundleData bundleToAssign,
LoadData loadData,
+ ServiceConfiguration conf)
Review Comment:
Can you clarify the purpose of these changes in this section? I don't think
we should remove the javadoc lines.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LeastLongTermMessageRate.java:
##########
@@ -37,12 +37,12 @@
* Placement strategy which selects a broker based on which one has the least
long term message rate.
*/
public class LeastLongTermMessageRate implements ModularLoadManagerStrategy {
- private static Logger log =
LoggerFactory.getLogger(LeastLongTermMessageRate.class);
+ private static final Logger log =
LoggerFactory.getLogger(LeastLongTermMessageRate.class);
// Maintain this list to reduce object creation.
private ArrayList<String> bestBrokers;
- public LeastLongTermMessageRate(final ServiceConfiguration conf) {
+ public LeastLongTermMessageRate() {
Review Comment:
Are you confident that this constructor is not used? I'd prefer to annotate
it as deprecated and create a new one just in case there are any custom load
balancer implementations relying on this constructor.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LoadManagerShared.java:
##########
@@ -486,7 +485,7 @@ public static CompletableFuture<Map<String, Integer>>
getAntiAffinityNamespaceOw
* @throws Exception
*/
public static boolean shouldAntiAffinityNamespaceUnload(
- String namespace, String bundle, String currentBroker,
Review Comment:
Similarly, I'd rather annotate the method as deprecated and then remove
later, since the load balancing logic is pluggable--unless we're confident no
one is using this method.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2792,7 +2790,8 @@ private TopicName validateTopicName(String topic, long
requestId, Object request
}
public ByteBufPair newMessageAndIntercept(long consumerId, long ledgerId,
long entryId, int partition,
- int redeliveryCount, ByteBuf metadataAndPayload, long[] ackSet,
String topic, long epoch) {
Review Comment:
I don't believe we should remove this method for the same reasons as above.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/PulsarServiceUnit.java:
##########
@@ -29,16 +29,16 @@ public class PulsarServiceUnit extends ServiceUnit {
private final PulsarServiceRequest srvRequest;
private ResourceDescription resrcRequired;
- public static PulsarServiceUnit parse(String suReqJson) {
Review Comment:
I was going to make the same comment as above, but it appears that this
class is not used. I'm not sure if we should delete it, though.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
