[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Only one `PubSub` is created for the lifetime of the adapter. Each `ExecutionContextHandler` receives a reference to it. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Can we then just add an interface and an implementation... It is easier to do this BEFORE rather than after. Also it makes it easier to achieve loose coupling. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
I wonder if one should expose the `PubSub` world here... maybe have the context have a `publish` method, which then delegates to the internal `PubSub`. To me it is a little like feature envy... Where instead of having the `ExecutionHandlerContext` handle the publish, we first have to know that there is a `PubSub` and then ask it to do work -- https://refactoring.guru/smells/feature-envy [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
We're coding to the interface and not the concrete class. It could easily be changed if the need arises. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Ditto above. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Same ditto [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
It's a good point. We realize that `context` is very overloaded right now and we're planning on refactoring in the future. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
I don't think that will work since `Comparable` only uses a single type and `isEqualTo` requires multiple types. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] gesterzhou commented on issue #4638: GEODE-7683: introduce BR.cmnClearRegion
there's some dunit tests fail from time to time on different test. I think it's unstable in base branch i.e feature/GEODE-7665 [ Full content available at: https://github.com/apache/geode/pull/4638 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Not really, but since this pattern is used throughout all the commands I think it would be better to make that change in a separate PR and cover all the commands. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Would we not prefer a checked exception here? [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Could we please raise one and address accordingly [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] igodwin closed pull request #4686: GEODE-6070: Fix ShutdownCommandOverHttpDUnitTest
[ pull request closed by igodwin ] [ Full content available at: https://github.com/apache/geode/pull/4686 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos opened pull request #4687: [WIP] GEODE-7789: Cache DurableClienAttributes
Keep the last known 'DurableClienAttributes' cached within the 'InternalDistributedMember' class to avoid the overhead of creating a new instance every time. 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: - [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [X] Is your initial contribution a single, squashed commit? - [X] Does `gradlew build` run cleanly? - [X] 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 d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/4687 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
It ensures that the `PubSub` instance is fully constructed before use - since the Function requires a reference to it. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Let me rephrase, given that there are no dependencies as input parameters, the registering of Functions can happen at construction time. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
That is true. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
I'm not sure I understand. We need to know two things: 1 - has a client already subscribed to a channel 2 - how many subscriptions a client currently has In order to address both of these concerns, the `findSubscribers` method would probably need to be inlined which seems like it would be less readable. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
I think that's just a different approach - also valid, but just different [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
So, you can 100% guarantee that the fields are ALWAYS non-null? [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
another approach is that the equals method is `this.channel.equals(channel`. This way the constructor can ensure that the instance channel field is non-null AND your `isEqualsTo` is null-safe [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] gesterzhou closed pull request #4638: GEODE-7683: introduce BR.cmnClearRegion
[ pull request closed by gesterzhou ] [ Full content available at: https://github.com/apache/geode/pull/4638 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
You are correct. That said.. What is the max number of Pub/Sub combinations we can expect? How does this approach scale? Maybe two maps > and > ? [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Fixed code [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
But I don't see any immediate value in changing it. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] rhoughton-pivot closed pull request #4685: GEODE-7788: Alpine tools docker image creation fails due to internal
[ pull request closed by rhoughton-pivot ] [ Full content available at: https://github.com/apache/geode/pull/4685 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Also, you can use `new PubSub` as the `registerFunction` can (and should) be called in the constructor of `PubSub` otherwise one is exposing knowledge that does not need to be exposed. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] bschuchardt opened pull request #4688: GEODE-7792: configure logging for geode-membership integration tests
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 d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/4688 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
Could we leave optimization until it's actually determined that it is needed? [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #4682: GEODE-7778: Add PUBLISH, SUBSCRIBE and UNSUBSCRIBE Redis commands
I agree, premature optimization But I'd be interested in what the "normal" usage on this would be. Maybe we add a load testing ticket on this, to see what load this construct can take. Is it 100's or 1000's or even 10,000's... But I think it would be good to understand when it will slow down / break. [ Full content available at: https://github.com/apache/geode/pull/4682 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[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 d...@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] jchen21 opened pull request #4690: GEODE-7746: onServers function throws a NPE if the distributed system is shutdown
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 d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/4690 ] 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
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] BenjaminPerryRoss commented on issue #4689: GEODE-7684: Create messaging for PR Clear
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
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
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
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
[GitHub] [geode] DonalEvans commented on pull request #4689: GEODE-7684: Create messaging for PR Clear
`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
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] jhuynh1 commented on pull request #4690: GEODE-7746: onServers function throws a NPE if the distributed system is shutdown
Also, is there a way we can convert to awaitility so there is no chance a test can hang forever? [ Full content available at: https://github.com/apache/geode/pull/4690 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal opened pull request #4692: Distributed rebalance status
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 d...@geode.apache.org. [ Full content available at: https://github.com/apache/geode/pull/4692 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
