upthewaterspout commented on a change in pull request #6808:
URL: https://github.com/apache/geode/pull/6808#discussion_r698624910
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/PartitionedRegionLoadModel.java
##########
@@ -470,12 +470,14 @@ public Move findBestRemove(Bucket bucket) {
Move bestMove = null;
for (Member member : bucket.getMembersHosting()) {
- float newLoad = (member.getTotalLoad() - bucket.getLoad()) /
member.getWeight();
- if (newLoad > mostLoaded && !member.equals(bucket.getPrimary())) {
- Move move = new Move(null, member, bucket);
- if (!this.attemptedBucketRemoves.contains(move)) {
- mostLoaded = newLoad;
- bestMove = move;
+ if (member.canDelete(bucket, this.partitionedRegion.getMyId(),
true).willAccept()) {
Review comment:
This seems to be the only call tot the new canDelete function? If so,
the third boolean parameter doesn't need to be there. Also it can just return a
boolean, since that is all you are using in the result. No need for a new
RefusalReason.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/RefusalReason.java
##########
@@ -42,6 +48,11 @@ public String formatMessage(Member target, Bucket bucket) {
case CRITICAL_HEAP:
return "Target member " + target.getMemberId()
+ " has reached its critical heap percentage, and cannot accept
more data";
+ case DIFFERENT_ZONE:
Review comment:
All of these other `RefusalReasons` are refusing to create a bucket. I
don't think DIFFERENT_ZONE belongs in this enum. Based on my ealier comment, I
think you can just remove it entirely since we don't use it.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/Member.java
##########
@@ -55,6 +55,31 @@
this.localMaxMemory = localMaxMemory;
}
+ /**
+ * @param sourceMember the member we will be moving this bucket off of
+ * @param checkZone true if we should not put two copies of a bucket on two
nodes with the same
+ * IP address.
+ */
+ public RefusalReason canDelete(Bucket bucket, InternalDistributedMember
sourceMember,
+ boolean checkZone) {
Review comment:
I'm not sure the logic in this method is correct. What if we have an
extra copy of the bucket, and each copy is in a different redundancy zone? It
seems like this would prevent us from deleting the extra copy when if fact we
should.
Maybe we could keep this method but only throw an exception if we find two
copies of the bucket in the same zone *and* we are trying to delete a copy in a
different zone, or something like that.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/PartitionedRegionLoadModel.java
##########
@@ -470,12 +470,14 @@ public Move findBestRemove(Bucket bucket) {
Move bestMove = null;
for (Member member : bucket.getMembersHosting()) {
- float newLoad = (member.getTotalLoad() - bucket.getLoad()) /
member.getWeight();
- if (newLoad > mostLoaded && !member.equals(bucket.getPrimary())) {
- Move move = new Move(null, member, bucket);
- if (!this.attemptedBucketRemoves.contains(move)) {
- mostLoaded = newLoad;
- bestMove = move;
+ if (member.canDelete(bucket, this.partitionedRegion.getMyId(),
true).willAccept()) {
Review comment:
This seems to be the only call to the new canDelete function? If so, the
third boolean parameter doesn't need to be there. Also it can just return a
boolean, since that is all you are using in the result. No need for a new
RefusalReason.
--
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]
