pivotal-jbarrett commented on a change in pull request #4909: URL: https://github.com/apache/geode/pull/4909#discussion_r412453438
########## File path: geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyResults.java ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.cache.control; + +import java.util.Map; + +import org.apache.geode.cache.partition.PartitionRebalanceInfo; +import org.apache.geode.internal.cache.control.RegionRedundancyStatus; +import org.apache.geode.internal.cache.partitioned.PartitionedRegionRebalanceOp; + +/** + * A class to collect the results of restore redundancy operations for one or more regions and + * determine the success of failure of the operation. + */ +public interface RestoreRedundancyResults { + + /** + * {@link #SUCCESS} is defined as every included region having fully satisfied redundancy. + * {@link #FAILURE} is defined as at least one region that is configured to have redundant copies + * having fewer than its configured number of redundant copies. + * {@link #ERROR} is for cases when the restore redundancy operation was unable to begin or threw + * an exception. + */ + enum Status { + SUCCESS, + FAILURE, + ERROR + } + + /** + * Adds the contents of another {@link RestoreRedundancyResults} object to this one, including + * both {@link RegionRedundancyStatus} objects and information on the number of primaries + * reassigned and the time taken to reassign them. + * + * @param results a {@link RestoreRedundancyResults} object whose contents will be added to this + * one. + */ + void addRegionResults(RestoreRedundancyResults results); + + /** + * Adds information regarding the number of primaries reassigned and the time taken to reassign + * them during a restore redundancy operation. + * + * @param details a {@link PartitionRebalanceInfo} generated by a + * {@link PartitionedRegionRebalanceOp} operation. Review comment: If we had a proper API module these would be a compile time failure for any tests. @upthewaterspout Seems like there should be a test in the static analyzer or other that should reject APIs like this. ########## File path: geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyResults.java ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.cache.control; + +import java.util.Map; + +import org.apache.geode.cache.partition.PartitionRebalanceInfo; +import org.apache.geode.internal.cache.control.RegionRedundancyStatus; +import org.apache.geode.internal.cache.partitioned.PartitionedRegionRebalanceOp; + +/** + * A class to collect the results of restore redundancy operations for one or more regions and + * determine the success of failure of the operation. + */ +public interface RestoreRedundancyResults { + + /** + * {@link #SUCCESS} is defined as every included region having fully satisfied redundancy. + * {@link #FAILURE} is defined as at least one region that is configured to have redundant copies + * having fewer than its configured number of redundant copies. + * {@link #ERROR} is for cases when the restore redundancy operation was unable to begin or threw + * an exception. + */ + enum Status { + SUCCESS, + FAILURE, + ERROR + } + + /** + * Adds the contents of another {@link RestoreRedundancyResults} object to this one, including + * both {@link RegionRedundancyStatus} objects and information on the number of primaries + * reassigned and the time taken to reassign them. + * + * @param results a {@link RestoreRedundancyResults} object whose contents will be added to this + * one. + */ + void addRegionResults(RestoreRedundancyResults results); + + /** + * Adds information regarding the number of primaries reassigned and the time taken to reassign + * them during a restore redundancy operation. + * + * @param details a {@link PartitionRebalanceInfo} generated by a + * {@link PartitionedRegionRebalanceOp} operation. + */ + void addPrimaryReassignmentDetails(PartitionRebalanceInfo details); + + /** + * Adds one {@link RegionRedundancyStatus} to the result set. + * + * @param regionResult The {@link RegionRedundancyStatus} to be added to the result set. + */ + void addRegionResult(RegionRedundancyStatus regionResult); + + /** + * Returns the {@link Status} of this restore redundancy operation. Possible statuses are + * {@link Status#SUCCESS}, {@link Status#FAILURE} and {@link Status#ERROR}. + * + * @return The {@link Status} of this restore redundancy operation. + */ + Status getStatus(); + + /** + * Returns a message describing the results of this restore redundancy operation. + * + * @return A {@link String} describing the results of this restore redundancy operation. + */ + String getMessage(); + + /** + * Returns the {@link RegionRedundancyStatus} for a specific region or null if that region + * is not present in this {@link RestoreRedundancyResults}. + * + * @param regionName The region to which the {@link RegionRedundancyStatus} to be returned + * belongs. + * @return A {@link RegionRedundancyStatus} for the specified region or null if that region is not + * present in this {@link RestoreRedundancyResults}. + */ + RegionRedundancyStatus getRegionResult(String regionName); + + /** + * Returns all the {@link RegionRedundancyStatus RegionRedundancyStatuses} for regions with + * configured redundancy but zero actual redundant copies. + * + * @return A {@link Map} of {@link String} region name to {@link RegionRedundancyStatus} for every + * region contained in this {@link RestoreRedundancyResults} with configured redundancy + * but zero actual redundant copies. + */ + Map<String, RegionRedundancyStatus> getZeroRedundancyRegionResults(); + + /** + * Returns all the {@link RegionRedundancyStatus RegionRedundancyStatuses} for regions with with + * at least one redundant copy, but fewer than the configured number of redundant copies. + * + * @return A {@link Map} of {@link String} region name to {@link RegionRedundancyStatus} for every + * region contained in this {@link RestoreRedundancyResults} with at least one redundant + * copy, but fewer than the configured number of redundant copies. + */ + Map<String, RegionRedundancyStatus> getUnderRedundancyRegionResults(); + + /** + * Returns all the {@link RegionRedundancyStatus RegionRedundancyStatuses} for regions with + * redundancy satisfied. + * + * @return A {@link Map} of {@link String} region name to {@link RegionRedundancyStatus} for every + * region contained in this {@link RestoreRedundancyResults} with redundancy satisfied. + */ + Map<String, RegionRedundancyStatus> getSatisfiedRedundancyRegionResults(); + + /** + * Returns all the {@link RegionRedundancyStatus RegionRedundancyStatuses} contained in this + * {@link RestoreRedundancyResults}. This method may return the actual backing map depending on + * implementation. + * + * @return A {@link Map} of {@link String} region name to {@link RegionRedundancyStatus} for every + * region contained in this {@link RestoreRedundancyResults}. + */ + Map<String, RegionRedundancyStatus> getRegionResults(); + + /** + * Returns the total number of primaries that were transferred as part of the restore redundancy + * operations. + * + * @return the total number of primaries that were transferred + */ + int getTotalPrimaryTransfersCompleted(); Review comment: Is it reasonable to assume we will never be able to transfer 2^31 or more primaries? ########## File path: geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyResults.java ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.cache.control; + +import java.util.Map; + +import org.apache.geode.cache.partition.PartitionRebalanceInfo; +import org.apache.geode.internal.cache.control.RegionRedundancyStatus; +import org.apache.geode.internal.cache.partitioned.PartitionedRegionRebalanceOp; + +/** + * A class to collect the results of restore redundancy operations for one or more regions and + * determine the success of failure of the operation. + */ +public interface RestoreRedundancyResults { + + /** + * {@link #SUCCESS} is defined as every included region having fully satisfied redundancy. + * {@link #FAILURE} is defined as at least one region that is configured to have redundant copies + * having fewer than its configured number of redundant copies. + * {@link #ERROR} is for cases when the restore redundancy operation was unable to begin or threw + * an exception. + */ + enum Status { + SUCCESS, + FAILURE, + ERROR + } + + /** + * Adds the contents of another {@link RestoreRedundancyResults} object to this one, including + * both {@link RegionRedundancyStatus} objects and information on the number of primaries + * reassigned and the time taken to reassign them. + * + * @param results a {@link RestoreRedundancyResults} object whose contents will be added to this + * one. + */ + void addRegionResults(RestoreRedundancyResults results); + + /** + * Adds information regarding the number of primaries reassigned and the time taken to reassign + * them during a restore redundancy operation. + * + * @param details a {@link PartitionRebalanceInfo} generated by a + * {@link PartitionedRegionRebalanceOp} operation. + */ + void addPrimaryReassignmentDetails(PartitionRebalanceInfo details); + + /** + * Adds one {@link RegionRedundancyStatus} to the result set. + * + * @param regionResult The {@link RegionRedundancyStatus} to be added to the result set. + */ + void addRegionResult(RegionRedundancyStatus regionResult); + + /** + * Returns the {@link Status} of this restore redundancy operation. Possible statuses are + * {@link Status#SUCCESS}, {@link Status#FAILURE} and {@link Status#ERROR}. + * + * @return The {@link Status} of this restore redundancy operation. + */ + Status getStatus(); + + /** + * Returns a message describing the results of this restore redundancy operation. + * + * @return A {@link String} describing the results of this restore redundancy operation. + */ + String getMessage(); + + /** + * Returns the {@link RegionRedundancyStatus} for a specific region or null if that region + * is not present in this {@link RestoreRedundancyResults}. + * + * @param regionName The region to which the {@link RegionRedundancyStatus} to be returned + * belongs. + * @return A {@link RegionRedundancyStatus} for the specified region or null if that region is not + * present in this {@link RestoreRedundancyResults}. + */ + RegionRedundancyStatus getRegionResult(String regionName); + + /** + * Returns all the {@link RegionRedundancyStatus RegionRedundancyStatuses} for regions with + * configured redundancy but zero actual redundant copies. + * + * @return A {@link Map} of {@link String} region name to {@link RegionRedundancyStatus} for every + * region contained in this {@link RestoreRedundancyResults} with configured redundancy + * but zero actual redundant copies. + */ + Map<String, RegionRedundancyStatus> getZeroRedundancyRegionResults(); + + /** + * Returns all the {@link RegionRedundancyStatus RegionRedundancyStatuses} for regions with with + * at least one redundant copy, but fewer than the configured number of redundant copies. + * + * @return A {@link Map} of {@link String} region name to {@link RegionRedundancyStatus} for every + * region contained in this {@link RestoreRedundancyResults} with at least one redundant + * copy, but fewer than the configured number of redundant copies. + */ + Map<String, RegionRedundancyStatus> getUnderRedundancyRegionResults(); + + /** + * Returns all the {@link RegionRedundancyStatus RegionRedundancyStatuses} for regions with + * redundancy satisfied. + * + * @return A {@link Map} of {@link String} region name to {@link RegionRedundancyStatus} for every + * region contained in this {@link RestoreRedundancyResults} with redundancy satisfied. + */ + Map<String, RegionRedundancyStatus> getSatisfiedRedundancyRegionResults(); + + /** + * Returns all the {@link RegionRedundancyStatus RegionRedundancyStatuses} contained in this + * {@link RestoreRedundancyResults}. This method may return the actual backing map depending on + * implementation. + * + * @return A {@link Map} of {@link String} region name to {@link RegionRedundancyStatus} for every + * region contained in this {@link RestoreRedundancyResults}. + */ + Map<String, RegionRedundancyStatus> getRegionResults(); + + /** + * Returns the total number of primaries that were transferred as part of the restore redundancy + * operations. + * + * @return the total number of primaries that were transferred + */ + int getTotalPrimaryTransfersCompleted(); + + /** + * Returns the total time, in milliseconds, spent transferring primaries as part of the restore + * redundancy operations. + * + * @return the total time, in milliseconds, spent transferring primaries + */ + long getTotalPrimaryTransferTime(); Review comment: We had discussion long ago about making sure time based APIs used `TimeUnit`. This method should be `long getTotalPrimaryTransferTime(TimeUnit unit)`. Alternatively we could consider `java.time.Duration`. Although that does incur an object allocation, given the infrequency that it is called the overhead is probably worth the correctness. ########## File path: geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyBuilder.java ########## @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.geode.cache.control; + +import java.util.Set; +import java.util.concurrent.CompletableFuture; + +/** + * Factory for defining and starting a {@link CompletableFuture} that returns + * {@link RestoreRedundancyResults}. + */ +public interface RestoreRedundancyBuilder { + /** + * Specify which regions to include in the restore redundancy operation. The default, + * <code>null<code>, means all regions should be included. Includes take precedence over + * excludes. + * + * @param regions A set containing the names of regions to include. + */ + RestoreRedundancyBuilder includeRegions(Set<String> regions); + + /** + * Exclude specific regions from the restore redundancy operation. The default, + * <code>null<code>, means don't exclude any regions. + * + * @param regions A set containing the names of regions to exclude. + */ + RestoreRedundancyBuilder excludeRegions(Set<String> regions); + + /** + * Set whether the restore redundancy operation should reassign primary buckets. The default, + * <code>true</code>, will result in primary buckets being reassigned for better load balancing + * across members. + * + * @param shouldReassign A boolean indicating whether or not the operation created by this + * class should reassign primary bucket hosts. + */ + RestoreRedundancyBuilder setReassignPrimaries(boolean shouldReassign); Review comment: I think `builder.shouldReassignPrimaries(true)` is more consistent with the other builder methods here. Otherwise I would change all to use the `set` prefix. Naming convention consistency is preferable. ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/control/InternalResourceManager.java ########## @@ -288,20 +294,20 @@ public RebalanceFactory createRebalanceFactory() { @Override public Set<RebalanceOperation> getRebalanceOperations() { - synchronized (this.inProgressOperationsLock) { - return new HashSet<RebalanceOperation>(this.inProgressOperations); + synchronized (this.inProgressRebalanceOperationsLock) { + return new HashSet<>(this.inProgressRebalanceOperations); Review comment: Maybe an unmodifiable set wrapped around a concurrent map? Also the use of `this.` qualifier is not necessary in this context. ########## File path: geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyBuilder.java ########## @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.geode.cache.control; + +import java.util.Set; +import java.util.concurrent.CompletableFuture; + +/** + * Factory for defining and starting a {@link CompletableFuture} that returns + * {@link RestoreRedundancyResults}. + */ +public interface RestoreRedundancyBuilder { + /** + * Specify which regions to include in the restore redundancy operation. The default, + * <code>null<code>, means all regions should be included. Includes take precedence over + * excludes. + * + * @param regions A set containing the names of regions to include. + */ + RestoreRedundancyBuilder includeRegions(Set<String> regions); + + /** + * Exclude specific regions from the restore redundancy operation. The default, + * <code>null<code>, means don't exclude any regions. + * + * @param regions A set containing the names of regions to exclude. + */ + RestoreRedundancyBuilder excludeRegions(Set<String> regions); + + /** + * Set whether the restore redundancy operation should reassign primary buckets. The default, + * <code>true</code>, will result in primary buckets being reassigned for better load balancing + * across members. + * + * @param shouldReassign A boolean indicating whether or not the operation created by this + * class should reassign primary bucket hosts. + */ + RestoreRedundancyBuilder setReassignPrimaries(boolean shouldReassign); + + /** + * Asynchronously starts a new restore redundancy operation. Only the cache resources used by this + * member will have redundancy restored. The operation may queue as needed for resources in + * contention by other active restore redundancy operations. + * + * @return a {@link CompletableFuture} which will return the results of the restore redundancy + * operation started by this method. + */ + CompletableFuture<RestoreRedundancyResults> start(); Review comment: A builder with a "start" method? I get that "build" doesn't feel right either. Maybe there is a better name for this class? `¯\_(ツ)_/¯` ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/control/RestoreRedundancyResultsImpl.java ########## @@ -0,0 +1,169 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.internal.cache.control; + +import java.io.Serializable; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +import org.apache.geode.cache.control.RestoreRedundancyResults; +import org.apache.geode.cache.partition.PartitionRebalanceInfo; + +public class RestoreRedundancyResultsImpl implements RestoreRedundancyResults, Serializable { Review comment: Again with the serialization here. ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/control/RegionRedundancyStatus.java ########## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.internal.cache.control; + +import java.io.Serializable; + +import org.apache.geode.internal.cache.PartitionedRegion; + +/** + * Used to calculate and store the redundancy status for a {@link PartitionedRegion}. + */ +public class RegionRedundancyStatus implements Serializable { Review comment: The only time I would say we should have Java Serializable classes is for testing. All other uses for client/server or p2p messaging should be `DataSerializableFixedId`. I wonder if our static analyzer could be updated to catch things like this? ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/control/RegionRedundancyStatus.java ########## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.internal.cache.control; + +import java.io.Serializable; + +import org.apache.geode.internal.cache.PartitionedRegion; + +/** + * Used to calculate and store the redundancy status for a {@link PartitionedRegion}. + */ +public class RegionRedundancyStatus implements Serializable { + private static final long serialVersionUID = 3407539362166634316L; + + public static final String OUTPUT_STRING = + "%s redundancy status: %s. Desired redundancy is %s and actual redundancy is %s."; + + /** + * The name of the region used to create this object. + */ + private final String regionName; + + /** + * The configured redundancy of the region used to create this object. + */ + private final int desiredRedundancy; + + /** + * The actual redundancy of the region used to create this object at time of creation. + */ + private final int actualRedundancy; + + /** + * The {@link RedundancyStatus} of the region used to create this object at time of creation. + */ + private final RedundancyStatus status; + + /** + * The redundancy status of the region used to create this object at time of creation. + * {@link #SATISFIED} if every bucket in the region has the configured number of redundant copies + * {@link #NOT_SATISFIED} if at least one bucket in the region has less than the configured number + * of redundant copies + * {@link #NO_REDUNDANT_COPIES} if at least one bucket in the region has zero redundant copies and + * the region is not configured for zero redundancy + */ + enum RedundancyStatus { + SATISFIED, + NOT_SATISFIED, + NO_REDUNDANT_COPIES + } + + public RegionRedundancyStatus(PartitionedRegion region) { + regionName = region.getName(); + desiredRedundancy = region.getRedundantCopies(); + actualRedundancy = calculateLowestRedundancy(region); + status = determineStatus(desiredRedundancy, actualRedundancy); + } + + public String getRegionName() { + return regionName; + } + + public int getActualRedundancy() { Review comment: Javadoc, what does "Actual Redundancy" mean? ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
