junkaixue commented on code in PR #2829:
URL: https://github.com/apache/helix/pull/2829#discussion_r1678455267
##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithm.java:
##########
@@ -136,11 +143,14 @@ private Optional<AssignableNode>
getNodeWithHighestPoints(AssignableReplica repl
}).collect(Collectors.toList());
if (candidateNodes.isEmpty()) {
+ enableFullLoggingForReplica(replica);
Review Comment:
Let's have a log here as warning?
##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/HardConstraint.java:
##########
@@ -29,6 +31,8 @@
*/
abstract class HardConstraint {
+ protected Set<String> logEnabledReplicas;
Review Comment:
As I mentioned, better have a flag for constraint level. Replica level
creates a lot of string objects for that. That's too much.
##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithm.java:
##########
@@ -136,11 +143,14 @@ private Optional<AssignableNode>
getNodeWithHighestPoints(AssignableReplica repl
}).collect(Collectors.toList());
if (candidateNodes.isEmpty()) {
+ enableFullLoggingForReplica(replica);
optimalAssignment.recordAssignmentFailure(replica,
Maps.transformValues(hardConstraintFailures,
this::convertFailureReasons));
return Optional.empty();
}
+ removeReplicaFromFullLogging(replica);
Review Comment:
Same here, we can have paired log to understand we exit it.
##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithm.java:
##########
@@ -57,11 +59,16 @@ class ConstraintBasedAlgorithm implements
RebalanceAlgorithm {
private static final Logger LOG =
LoggerFactory.getLogger(ConstraintBasedAlgorithm.class);
private final List<HardConstraint> _hardConstraints;
private final Map<SoftConstraint, Float> _softConstraints;
+ private final Set<String> logEnabledReplicas = new HashSet<>();
ConstraintBasedAlgorithm(List<HardConstraint> hardConstraints,
Map<SoftConstraint, Float> softConstraints) {
_hardConstraints = hardConstraints;
_softConstraints = softConstraints;
+
+ for (HardConstraint constraint : hardConstraints) {
+ constraint.setLogEnabledReplicas(logEnabledReplicas);
+ }
Review Comment:
We can do a if check if logEnabledReplicas is false, we can skip it. And we
should use false as default value.
Replica level is too much. We can just make it as constraint level for
entire cluster.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
