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 2021/09/29 01:22:12 UTC
[GitHub] [geode] agingade opened a new pull request #6914: GEODE-8200: After checking locator presence store the status in OperationStateStore
agingade opened a new pull request #6914:
URL: https://github.com/apache/geode/pull/6914
After checking locator presence store the status in OperationStateStore
### For all changes:
- [Y] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [Y] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [Y] Is your initial contribution a single, squashed commit?
- [Y] Does `gradlew build` run cleanly?
- [Y] Have you written or updated unit tests to verify your changes?
- [NA] 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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade merged pull request #6914: GEODE-8200: After checking locator presence store the status in OperationStateStore
Posted by GitBox <gi...@apache.org>.
agingade merged pull request #6914:
URL: https://github.com/apache/geode/pull/6914
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6914: GEODE-8200: After checking locator presence store the status in OperationStateStore
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6914:
URL: https://github.com/apache/geode/pull/6914#discussion_r718724159
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationHistoryManager.java
##########
@@ -96,10 +96,15 @@ private static boolean isExpired(long expirationTime, OperationState<?, ?> opera
}
private OperationState<?, ?> validateLocator(OperationState<?, ?> operationState) {
+ if (operationState.getOperationEnd() != null) {
+ return operationState;
+ }
if (isLocatorOffline(operationState)) {
operationState.setOperationEnd(new Date(), null,
new RuntimeException("Locator that initiated the Rest API operation is offline: "
+ operationState.getLocator()));
+ operationStateStore.recordEnd(operationState.getId(), operationState.getResult(),
Review comment:
I think you should call "recordEnd" here instead of "operationStateStore.recordEnd". Also it looks like recordEnd will in turn call operationState.setOperationEnd. So I think this whole if block could just be this one call: recordEnd(new Date(), null,
new RuntimeException("Locator that initiated the Rest API operation is offline: "
+ operationState.getLocator()));
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6914: GEODE-8200: After checking locator presence store the status in OperationStateStore
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6914:
URL: https://github.com/apache/geode/pull/6914#discussion_r719749264
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationHistoryManager.java
##########
@@ -95,13 +95,13 @@ 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,
+ private OperationState<?, ?> validateLocatorAndUpdateOperationState(
+ OperationState<?, ?> operationState) {
+ if (operationState.getOperationEnd() == null && isLocatorOffline(operationState)) {
Review comment:
get rid of the "getOperationEnd() == null" check here. It is already in isLocatorOffline.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #6914: GEODE-8200: After checking locator presence store the status in OperationStateStore
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #6914:
URL: https://github.com/apache/geode/pull/6914#discussion_r718964365
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationHistoryManager.java
##########
@@ -96,10 +96,15 @@ private static boolean isExpired(long expirationTime, OperationState<?, ?> opera
}
private OperationState<?, ?> validateLocator(OperationState<?, ?> operationState) {
+ if (operationState.getOperationEnd() != null) {
+ return operationState;
+ }
if (isLocatorOffline(operationState)) {
operationState.setOperationEnd(new Date(), null,
new RuntimeException("Locator that initiated the Rest API operation is offline: "
+ operationState.getLocator()));
+ operationStateStore.recordEnd(operationState.getId(), operationState.getResult(),
Review comment:
Changes are made as suggested. And the method name is changed to reflect the value is recorded.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #6914: GEODE-8200: After checking locator presence store the status in OperationStateStore
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #6914:
URL: https://github.com/apache/geode/pull/6914#discussion_r719789472
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationHistoryManager.java
##########
@@ -95,13 +95,13 @@ 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,
+ private OperationState<?, ?> validateLocatorAndUpdateOperationState(
+ OperationState<?, ?> operationState) {
+ if (operationState.getOperationEnd() == null && isLocatorOffline(operationState)) {
Review comment:
Good catch @dschneider-pivotal
Removed the "operationState.getOperationEnd() == null" in isLocatorOffline()
Thanks.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #6914: GEODE-8200: After checking locator presence store the status in OperationStateStore
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #6914:
URL: https://github.com/apache/geode/pull/6914#discussion_r718964365
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationHistoryManager.java
##########
@@ -96,10 +96,15 @@ private static boolean isExpired(long expirationTime, OperationState<?, ?> opera
}
private OperationState<?, ?> validateLocator(OperationState<?, ?> operationState) {
+ if (operationState.getOperationEnd() != null) {
+ return operationState;
+ }
if (isLocatorOffline(operationState)) {
operationState.setOperationEnd(new Date(), null,
new RuntimeException("Locator that initiated the Rest API operation is offline: "
+ operationState.getLocator()));
+ operationStateStore.recordEnd(operationState.getId(), operationState.getResult(),
Review comment:
Changes are made as suggested. And the method name is changed to reflect the value is recorded.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6914: GEODE-8200: After checking locator presence store the status in OperationStateStore
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6914:
URL: https://github.com/apache/geode/pull/6914#discussion_r718724159
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/OperationHistoryManager.java
##########
@@ -96,10 +96,15 @@ private static boolean isExpired(long expirationTime, OperationState<?, ?> opera
}
private OperationState<?, ?> validateLocator(OperationState<?, ?> operationState) {
+ if (operationState.getOperationEnd() != null) {
+ return operationState;
+ }
if (isLocatorOffline(operationState)) {
operationState.setOperationEnd(new Date(), null,
new RuntimeException("Locator that initiated the Rest API operation is offline: "
+ operationState.getLocator()));
+ operationStateStore.recordEnd(operationState.getId(), operationState.getResult(),
Review comment:
I think you should call "recordEnd" here instead of "operationStateStore.recordEnd". Also it looks like recordEnd will in turn call operationState.setOperationEnd. So I think this whole if block could just be this one call: recordEnd(new Date(), null,
new RuntimeException("Locator that initiated the Rest API operation is offline: "
+ operationState.getLocator()));
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org