Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GEODE-2999: Add PutIfAbsent to the Protobuf protocol. #1578

Merged
merged 1 commit into from Mar 13, 2018
Merged

GEODE-2999: Add PutIfAbsent to the Protobuf protocol. #1578

merged 1 commit into from Mar 13, 2018

Conversation

galen-pivotal
Copy link
Contributor

  • Some test cleanup
  • Add it to the experimental protobuf client driver

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?

Note:

Please ensure that once the PR is submitted, you check travis-ci 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.

upthewaterspout
upthewaterspout requested changes Mar 7, 2018
View reviewed changes
Copy link
Contributor

@upthewaterspout upthewaterspout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should putIfAbsent return the existing value, like the server side API? Or should we call this operation create instead, which is a server side API that does this same thing but doesn't return the existing value.

...tobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufOpsProcessor.java Outdated
@@ -86,6 +85,9 @@ private Result processOperation(ClientProtocol.Message request, MessageExecution
logger.error(exception);
return Failure.of(BasicTypes.ErrorCode.INVALID_REQUEST,
"Invalid execution context found for operation.");
} catch (UnsupportedOperationException ex) {
return Failure.of(BasicTypes.ErrorCode.UNSUPPORTED_OPERATION,
"Unsupported operation. This may be due to a region type that does not support that operation.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we return the message of the underlying exception to the user, maybe? I suspect a lot of UnsupportedOperationExceptions are not thrown from Region. Also, we should still log the stack trace I think. Otherwise we may never be able to track down what method actually threw UnsupportedOperation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done.

WireBaron
WireBaron requested changes Mar 8, 2018
View reviewed changes
...pache/geode/internal/protocol/protobuf/v1/operations/PutIfAbsentRequestOperationHandler.java Outdated
public static ResourcePermission determineRequiredPermission(RegionAPI.PutIfAbsentRequest request,
ProtobufSerializationService serializer) throws DecodingException {
return new ResourcePermission(ResourcePermission.Resource.DATA,
ResourcePermission.Operation.READ, request.getRegionName(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a WRITE permission.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, thanks. Good catch.

.../java/org/apache/geode/internal/protocol/protobuf/v1/utilities/ProtobufRequestUtilities.java Outdated
* @return Request object containing the passed params.
*/
public static ClientProtocol.Message createPutIfAbsentRequest(String region,
BasicTypes.Entry entry) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do these utilities exist in the src side? I would think this would only be useful for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had meant to put this in MessageUtil. My bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I misread this. I had thought I'd put the method in the wrong place but in fact this class is only used for tests. I'll move it over.

@galen-pivotal
Copy link
Contributor Author

@upthewaterspout I think you're right about using create instead of putIfAbsent, or serializing the value with putIfAbsent as we have it currently. I'll change this to return the current value if it doesn't replace.

@galen-pivotal galen-pivotal force-pushed the feature/GEODE-2999 branch 3 times, most recently from bf2cf14 to fc30b12 Compare March 9, 2018 19:08
@galen-pivotal
Copy link
Contributor Author

@upthewaterspout / @WireBaron Can you re-review this, please?

@galen-pivotal
GEODE-2999: Add PutIfAbsent to the Protobuf protocol.
7dcb732 
* Some test cleanup
* Add it to the experimental protobuf client driver
@galen-pivotal
Copy link
Contributor Author

rebased on develop.

@galen-pivotal galen-pivotal merged commit 6214a43 into apache:develop Mar 13, 2018
galen-pivotal added a commit that referenced this pull request Mar 14, 2018
@galen-pivotal
Revert "GEODE-2999: Add PutIfAbsent to the Protobuf protocol. (#1578)"
7a3da49 
This reverts commit 6214a43.

Hopefully this fixes CI and can be pushed after.
@galen-pivotal galen-pivotal mentioned this pull request Mar 15, 2018
Closed
6 tasks
pdxrunner pushed a commit to pdxrunner/geode that referenced this pull request Mar 19, 2018
@galen-pivotal
GEODE-2999: Add PutIfAbsent to the Protobuf protocol. (apache#1578)
8db6377 
* Some test cleanup
* Add it to the experimental protobuf client driver
pdxrunner pushed a commit to pdxrunner/geode that referenced this pull request Mar 19, 2018
@galen-pivotal
Revert "GEODE-2999: Add PutIfAbsent to the Protobuf protocol. (apache…
556e4da 
…#1578)"

This reverts commit 6214a43.

Hopefully this fixes CI and can be pushed after.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upthewaterspout upthewaterspout upthewaterspout approved these changes

@bschuchardt bschuchardt bschuchardt approved these changes

@WireBaron WireBaron WireBaron approved these changes

@PivotalSarge PivotalSarge PivotalSarge approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@galen-pivotal @upthewaterspout @bschuchardt @WireBaron @PivotalSarge