You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/07/07 00:43:22 UTC
[GitHub] [geode] jchen21 opened a new pull request #5350: GEODE-8200: Rebalance operations stuck in "IN_PROGRESS" state forever
jchen21 opened a new pull request #5350:
URL: https://github.com/apache/geode/pull/5350
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5350: GEODE-8200: Rebalance operations stuck in "IN_PROGRESS" state forever
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5350:
URL: https://github.com/apache/geode/pull/5350#discussion_r452534574
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationHistoryManager.java
##########
@@ -90,6 +95,27 @@ private static boolean isExpired(long expirationTime, OperationState<?, ?> opera
return operationEnd.getTime() <= expirationTime;
}
+ private OperationState<?, ?> validateLocator(OperationState<?, ?> operationState) {
+ if (isLocatorOffline(operationState)) {
+ operationState.setOperationEnd(new Date(), null,
+ new RuntimeException("Locator that initiated the Rest API operation is offline: "
+ + operationState.getLocator()));
+ }
+
+ return operationState;
+ }
+
+ private boolean isLocatorOffline(OperationState operationState) {
+ if (operationState.getOperationEnd() == null
+ && (operationState.getLocator() != null)
+ && cache.getMyId().toString().compareTo(operationState.getLocator()) != 0
Review comment:
Good point!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5350: GEODE-8200: Rebalance operations stuck in "IN_PROGRESS" state forever
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5350:
URL: https://github.com/apache/geode/pull/5350#discussion_r451826206
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationState.java
##########
@@ -28,12 +28,25 @@
*/
public class OperationState<A extends ClusterManagementOperation<V>, V extends OperationResult>
implements Identifiable<String> {
+ private static final long serialVersionUID = 8212319653561969588L;
private final String opId;
private final A operation;
private final Date operationStart;
private Date operationEnd;
private V result;
private Throwable throwable;
+ private String locator;
+
+ public String getLocator() {
+ return this.locator;
+ }
+
+ public void setLocator(
+ String locator) {
+ synchronized (this) {
Review comment:
this is just one line operation, is this not atomic? If not, can we put the synchronize on the method?
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationStateStore.java
##########
@@ -53,6 +53,8 @@
*/
<V extends OperationResult> void recordEnd(String opId, V result, Throwable exception);
+ void recordLocator(String opId, String locator);
Review comment:
instead of adding this interface, you can change the method of recordStart() to add the a locator id parameter, since when started, we should always know what locator started this operation.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #5350: GEODE-8200: Rebalance operations stuck in "IN_PROGRESS" state forever
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5350:
URL: https://github.com/apache/geode/pull/5350#discussion_r452518577
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationHistoryManager.java
##########
@@ -90,6 +95,27 @@ private static boolean isExpired(long expirationTime, OperationState<?, ?> opera
return operationEnd.getTime() <= expirationTime;
}
+ private OperationState<?, ?> validateLocator(OperationState<?, ?> operationState) {
+ if (isLocatorOffline(operationState)) {
+ operationState.setOperationEnd(new Date(), null,
+ new RuntimeException("Locator that initiated the Rest API operation is offline: "
+ + operationState.getLocator()));
+ }
+
+ return operationState;
+ }
+
+ private boolean isLocatorOffline(OperationState operationState) {
+ if (operationState.getOperationEnd() == null
+ && (operationState.getLocator() != null)
+ && cache.getMyId().toString().compareTo(operationState.getLocator()) != 0
Review comment:
Does it need to be compared? can it be changed to "equals"....
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationState.java
##########
@@ -28,12 +28,25 @@
*/
public class OperationState<A extends ClusterManagementOperation<V>, V extends OperationResult>
implements Identifiable<String> {
+ private static final long serialVersionUID = 8212319653561969588L;
private final String opId;
private final A operation;
private final Date operationStart;
private Date operationEnd;
private V result;
private Throwable throwable;
+ private String locator;
Review comment:
Can this be DistributedID than a String ID. That way we can avoid converting to string in other places?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5350: GEODE-8200: Rebalance operations stuck in "IN_PROGRESS" state forever
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5350:
URL: https://github.com/apache/geode/pull/5350#discussion_r452488277
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationState.java
##########
@@ -28,12 +28,25 @@
*/
public class OperationState<A extends ClusterManagementOperation<V>, V extends OperationResult>
implements Identifiable<String> {
+ private static final long serialVersionUID = 8212319653561969588L;
private final String opId;
private final A operation;
private final Date operationStart;
private Date operationEnd;
private V result;
private Throwable throwable;
+ private String locator;
+
+ public String getLocator() {
+ return this.locator;
+ }
+
+ public void setLocator(
+ String locator) {
+ synchronized (this) {
Review comment:
This is for consistency. `setOperationEnd()` does the same.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationStateStore.java
##########
@@ -53,6 +53,8 @@
*/
<V extends OperationResult> void recordEnd(String opId, V result, Throwable exception);
+ void recordLocator(String opId, String locator);
Review comment:
Good point.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5350: GEODE-8200: Rebalance operations stuck in "IN_PROGRESS" state forever
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5350:
URL: https://github.com/apache/geode/pull/5350#discussion_r452537292
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationState.java
##########
@@ -28,12 +28,25 @@
*/
public class OperationState<A extends ClusterManagementOperation<V>, V extends OperationResult>
implements Identifiable<String> {
+ private static final long serialVersionUID = 8212319653561969588L;
private final String opId;
private final A operation;
private final Date operationStart;
private Date operationEnd;
private V result;
private Throwable throwable;
+ private String locator;
Review comment:
Do you mean `InternalDistributedMember` when you talk about `DistributedID`? I was using the `InternalDistributedMember` as the type for `locator` field. But it is a large object with many fields. We only need to identify the locator, so a String type of a member's ID should be good.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jchen21 merged pull request #5350: GEODE-8200: Rebalance operations stuck in "IN_PROGRESS" state forever
Posted by GitBox <gi...@apache.org>.
jchen21 merged pull request #5350:
URL: https://github.com/apache/geode/pull/5350
----------------------------------------------------------------
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:
users@infra.apache.org