You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "BenjaminPerryRoss (GitHub)" <gi...@apache.org> on 2020/02/11 23:12:58 UTC

[GitHub] [geode] BenjaminPerryRoss opened pull request #4689: GEODE-7684: Create messaging for PR Clear

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.


[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "BenjaminPerryRoss (GitHub)" <gi...@apache.org>.
Done.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on issue #4689: GEODE-7684: Create messaging for PR Clear

Posted by "BenjaminPerryRoss (GitHub)" <gi...@apache.org>.
After talking with Gester - we should remove notify only and the transactional related stuff, make sure that doLocalClear() throws ForceReattemptException instead of PartitionedRegionException, and use a while loop to get the lock/check primary (see lucine IndexRepositoryFactory.java)

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.
`this.isSevereAlertCompatible()` is always true for this class, so it can probably be removed from this if statement.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.
Since they're `boolean` you don't need to explicitly set them to false; they default to that value even if they're not initialized, so we can avoid cluttering the constructor with assignments that are never used by the class.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on issue #4689: GEODE-7684: Create messaging for PR Clear

Posted by "BenjaminPerryRoss (GitHub)" <gi...@apache.org>.
After talking with Gester - we should remove notify only, transactional related stuff, and notifyGatewaySender logic, make sure that doLocalClear() throws ForceReattemptException instead of PartitionedRegionException, and use a while loop to get the lock/check primary (see lucine IndexRepositoryFactory.java)

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.
Remove comment.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "BenjaminPerryRoss (GitHub)" <gi...@apache.org>.
This is the same as the posDup above, always false, but needed by the parent. I've set it to false in the constructor as well.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.
This is currently never used, so unless it's going to be needed for the API story, it should probably be removed.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.
I think this comment is probably no longer needed, as it was originally just a placeholder and the thing it's describing is very self-explanatory.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "BenjaminPerryRoss (GitHub)" <gi...@apache.org>.
Done.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "BenjaminPerryRoss (GitHub)" <gi...@apache.org>.
Done.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "BenjaminPerryRoss (GitHub)" <gi...@apache.org>.
Done.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.
This is set in the constructor and used in `toData()` and `fromData()` but other than that it's never accessed or modified, so it can probably be removed.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.
There's an extra line here that could be tidied up.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "BenjaminPerryRoss (GitHub)" <gi...@apache.org>.
Done.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4689: GEODE-7684: Create messaging for PR Clear

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.
It might be worth including tests to confirm that `initMessage()` correctly sets the `processorId` and doesn't call `enableSevereAlertProcessing()` on the `ReplyProcessor` if the processor is null, a test to confirm that `send()` throws an exception if `putOutgoing(this)` returns a non-null set, a test to confirm that `operateOnPartitionedRegion()` calls `sendReply()` with the correct arguments if a `ForceReattemptException` is thrown during `doLocalClear()`, a test to confirm that we call `endPartitionMessagesProcessing()` in `sendReply()` when appropriate and a test to confirm that we only call `replyProcessor.process()` in `ClearReplyMessage.process()` when the replyProcessor is not null.

[ Full content available at: https://github.com/apache/geode/pull/4689 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org