[GitHub] [geode] DonalEvans commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command

2020-06-18 Thread GitBox


DonalEvans commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r442536212



##
File path: 
geode-management/src/main/java/org/apache/geode/management/runtime/RestoreRedundancyResults.java
##
@@ -18,28 +18,28 @@
 import java.util.List;
 import java.util.Map;
 
+import org.apache.geode.annotations.Experimental;
+
 /**
  * A class to collect the results of restore redundancy operations for one or 
more regions and
  * determine the success of failure of the operation.
  */
+@Experimental
 public interface RestoreRedundancyResults extends OperationResult {
 
   /**
* {@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

Review comment:
   You're correct, this was originally intended to be a way to capture the 
case that an exception was thrown during the  `doRestoreRedundancy()` method in 
`RestoreRedundancyOperationImpl`, but in order to keep the implementation as 
similar to that found in `RebalanceOperationImpl` as possible, this was not 
implemented. It should be fine to 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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [geode] DonalEvans commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command

2020-06-17 Thread GitBox


DonalEvans commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441873906



##
File path: 
geode-core/src/main/java/org/apache/geode/management/internal/operation/RestoreRedundancyPerformer.java
##
@@ -112,14 +114,14 @@ public RestoreRedundancyResults perform(Cache cache, 
RestoreRedundancyRequest op
 return finalResult;
   }
 
-  // this returns either an Exception or RestoreRedundancyResults
+  // this returns RestoreRedundancyResults or null based on

Review comment:
   Incomplete comment here?





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




[GitHub] [geode] DonalEvans commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command

2020-06-17 Thread GitBox


DonalEvans commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441860267



##
File path: 
geode-core/src/test/java/org/apache/geode/management/internal/functions/RestoreRedundancyFunctionTest.java
##
@@ -0,0 +1,163 @@
+/*
+ * 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.management.internal.functions;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.control.RestoreRedundancyOperation;
+import org.apache.geode.cache.execute.FunctionContext;
+import org.apache.geode.cache.execute.ResultSender;
+import 
org.apache.geode.internal.cache.control.SerializableRestoreRedundancyResultsImpl;
+import org.apache.geode.management.operation.RestoreRedundancyRequest;
+import org.apache.geode.management.runtime.RestoreRedundancyResults;
+
+public class RestoreRedundancyFunctionTest {
+  @SuppressWarnings("unchecked")
+  private final FunctionContext mockContext = 
mock(FunctionContext.class);
+  private final Cache mockCache = mock(Cache.class, RETURNS_DEEP_STUBS);
+  private final RestoreRedundancyOperation mockOperation =
+  mock(RestoreRedundancyOperation.class, RETURNS_DEEP_STUBS);
+  private final SerializableRestoreRedundancyResultsImpl mockResults =
+  mock(SerializableRestoreRedundancyResultsImpl.class);
+  private final String message = "expected message";
+  private RestoreRedundancyFunction function;
+  private ResultSender resultSender;
+  private ArgumentCaptor 
argumentCaptor;
+  private RestoreRedundancyRequest request;
+
+  @Before
+  public void setUp() throws InterruptedException, ExecutionException {
+function = new RestoreRedundancyFunction();
+when(mockContext.getCache()).thenReturn(mockCache);
+request = new RestoreRedundancyRequest();

Review comment:
   If we can be reasonably sure that this will remain true, and that no 
additional logic will be added to `RestoreRedundancyRequest` in the future, 
then this is fine to leave as it is.





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