Re: Avoid PowerMockito
Turns out I should have refreshed the ticket comments before submitting my PR, because Michael Oleske beat me to it yesterday: https://github.com/apache/geode/pull/6107 Sorry for the email spam! From: Donal Evans Sent: Wednesday, March 10, 2021 11:55 AM To: dev@geode.apache.org Subject: Re: Avoid PowerMockito I've just submitted a PR to remove three uses of PowerMock.when that can be trivially replaced with Mockito.when: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6112data=04%7C01%7Cdoevans%40vmware.com%7C8dc2e892bddd4165b16e08d8e3fe903d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C637510029773259071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=wP7iJXJzYV6fkl%2FXzOmVbkp%2BhcH4eL0ZhrYf4lks0Oo%3Dreserved=0 and updated the ticket with a comment listing the remaining classes that are using PowerMock. Only 7 files remain, so this feels like a very achievable task now. From: Kirk Lund Sent: Tuesday, March 9, 2021 12:13 PM To: dev@geode.apache.org Subject: Avoid PowerMockito I just reviewed a PR that was adding a unit test using PowerMockito. We've had lots of problems with PowerMockito leaving the unit testing JVM corrupted for later tests. Using PowerMockito also discourages us from refactoring product code to have better design and be easier to unit test. So in previous threads here on dev-list, the community decided to axe our usage of PowerMockito. There are lots of Jira tickets about PowerMockito in Geode: https://nam04.safelinks.protection.outlook.com/?url=https:%2F%2Fissues.apache.org%2Fjira%2Fissues%2F%3Fjql%3Dproject%2520%253D%2520GEODE%2520AND%2520text%2520~%2520%2522PowerMockito%2522data=04%7C01%7Cdoevans%40vmware.com%7C8dc2e892bddd4165b16e08d8e3fe903d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C637510029773259071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=mQ43v09i2hfiwumibgekykyNu%2FdXlpmn9ZDG41R0Tko%3Dreserved=0 There is one open ticket for removing PowerMockito from Geode: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissues.apache.org%2Fjira%2Fbrowse%2FGEODE-6143data=04%7C01%7Cdoevans%40vmware.com%7C8dc2e892bddd4165b16e08d8e3fe903d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C637510029773259071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=%2B1arhtioR6OiHuFYe2AlB6CkO8CUqtIYH2GyZTP30Iw%3Dreserved=0 Unfortunately the assignee is no longer active in this community so we need someone or everyone to pitch in. If you find yourself working on an area of code that has a unit test using PowerMockito, please rewrite the test using regular Mockito and refactor the code that it tests so that you can pass all of its dependencies in via the constructor(s). If anyone would like to volunteer to take on GEODE-6143, please feel free to reassign it. I would be happy to help you out. Thanks, Kirk
Re: Avoid PowerMockito
I've just submitted a PR to remove three uses of PowerMock.when that can be trivially replaced with Mockito.when: https://github.com/apache/geode/pull/6112 and updated the ticket with a comment listing the remaining classes that are using PowerMock. Only 7 files remain, so this feels like a very achievable task now. From: Kirk Lund Sent: Tuesday, March 9, 2021 12:13 PM To: dev@geode.apache.org Subject: Avoid PowerMockito I just reviewed a PR that was adding a unit test using PowerMockito. We've had lots of problems with PowerMockito leaving the unit testing JVM corrupted for later tests. Using PowerMockito also discourages us from refactoring product code to have better design and be easier to unit test. So in previous threads here on dev-list, the community decided to axe our usage of PowerMockito. There are lots of Jira tickets about PowerMockito in Geode: https://nam04.safelinks.protection.outlook.com/?url=https:%2F%2Fissues.apache.org%2Fjira%2Fissues%2F%3Fjql%3Dproject%2520%253D%2520GEODE%2520AND%2520text%2520~%2520%2522PowerMockito%2522data=04%7C01%7Cdoevans%40vmware.com%7C5ab7c4badf1342ceb86408d8e337e25e%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C637509176457807900%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=MUmrWogUil8YVFdFDhq8x9K%2B6%2F3C%2Fri6avfIZL0RnOI%3Dreserved=0 There is one open ticket for removing PowerMockito from Geode: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissues.apache.org%2Fjira%2Fbrowse%2FGEODE-6143data=04%7C01%7Cdoevans%40vmware.com%7C5ab7c4badf1342ceb86408d8e337e25e%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C637509176457807900%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=3BMSkWfQ%2BU%2BOuhkSheMRGRZlg65usvgSl%2Brv0635Wnk%3Dreserved=0 Unfortunately the assignee is no longer active in this community so we need someone or everyone to pitch in. If you find yourself working on an area of code that has a unit test using PowerMockito, please rewrite the test using regular Mockito and refactor the code that it tests so that you can pass all of its dependencies in via the constructor(s). If anyone would like to volunteer to take on GEODE-6143, please feel free to reassign it. I would be happy to help you out. Thanks, Kirk
Re: Question regarding VersionRequest/VersionResponse messages
Rolling upgrade is only supported for Geode v1.0 and up. Rolling from GemFire
8 isn't supported due to the JGroups upgrade.
On 3/10/21, 10:37 AM, "Bill Burcham" wrote:
Jakov, VersionRequest/VersionResponse is understood by the 8.1.x/GFE_81
product versions.
I don't know what Geode's stance is on supporting old locator versions. I
see the recent:
GEODE-8837 - Establishes GFE_81 as the oldest supported client.
Does that imply that GFE_81 is now the oldest supported version for
locators in rolling upgrade too? Seems reasonable to me. What do others
think?
On Tue, Mar 9, 2021 at 12:37 AM Jakov Varenina
wrote:
> Hi community,
>
> I have one question regarding VersionRequest/VersionResponse messages.
>
> Before member sends actual message, it has to first determine the remote
> member version. This is done by exchanging
> /VersionRequest///VersionResponse/ messages using function
> /getServerVersion() /from class /TcpClient.java/. There is part of code
> in /getServerVersion()/ for which I'm unsure which case is actually
> covering:
>
>
>
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fblob%2F854456c81ca7b9545eba252b6fa075318bb33af8%2Fgeode-tcp-server%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgeode%2Fdistributed%2Finternal%2Ftcpserver%2FTcpClient.java%23L289data=04%7C01%7Cbruces%40vmware.com%7C5b82c9992ccc4a75da8008d8e3f39254%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C637509982565124714%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=AJsl8YYEqvt%2BFGFPKAAY2yRNIoezZZR4OalzAuW7wQk%3Dreserved=0
>
>
> try {
>final Object readObject =objectDeserializer.readObject(versionedIn); if
> (!(readObjectinstanceof VersionResponse)) {
> throw new IllegalThreadStateException(
> "Server version response invalid: This could be the result of
> trying to
> connect a non-SSL-enabled client to an SSL-enabled locator."); }
>
>final VersionResponse response = (VersionResponse) readObject;
> serverVersion = response.getVersionOrdinal(); serverVersions.put(address,
> serverVersion); return serverVersion; }catch (EOFException ex) {
>// old locators will not recognize the version request and will close
> the connection }
> ...
> return KnownVersion.OLDEST.ordinal();
>
> The case is when /readObject()/ try to read /VersionResponse/ and then
> throws /EOFException/. As you can see, there is comment in catch
> statement explaining the case, but I'm not sure that I understand it
> correctly. What I assume is that member with old version (less or equal
> to /KnownVersion.OLDEST/) of Geode does not support /VersionRequest/
> message, and will close connection if receive such message. This will
> result with /EOFException/ on remote member that sent /VersionRequest/.
> Is this correct understanding? Is this case still valid with the latest
> non-deprecated version set to /GFE_81/?
>
> The reason why I'm asking this is because in some cases in kuberentes
> cluster, it is possible to get /EOFException/ when remote member is not
> yet accessible. In this case member will still try to send message (e.g.
> /RemoteLocatorJoinRequest/) thinking that it is old member on the other
> side, and that will then result with /ClassCastException/ when reading
> response (e.g. /RemoteLocatorJoinResponse/).
>
> BRs,
>
> Jakov
>
>
Re: Question regarding VersionRequest/VersionResponse messages
Jakov, VersionRequest/VersionResponse is understood by the 8.1.x/GFE_81
product versions.
I don't know what Geode's stance is on supporting old locator versions. I
see the recent:
GEODE-8837 - Establishes GFE_81 as the oldest supported client.
Does that imply that GFE_81 is now the oldest supported version for
locators in rolling upgrade too? Seems reasonable to me. What do others
think?
On Tue, Mar 9, 2021 at 12:37 AM Jakov Varenina
wrote:
> Hi community,
>
> I have one question regarding VersionRequest/VersionResponse messages.
>
> Before member sends actual message, it has to first determine the remote
> member version. This is done by exchanging
> /VersionRequest///VersionResponse/ messages using function
> /getServerVersion() /from class /TcpClient.java/. There is part of code
> in /getServerVersion()/ for which I'm unsure which case is actually
> covering:
>
>
> https://github.com/apache/geode/blob/854456c81ca7b9545eba252b6fa075318bb33af8/geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpClient.java#L289
>
>
> try {
>final Object readObject =objectDeserializer.readObject(versionedIn); if
> (!(readObjectinstanceof VersionResponse)) {
> throw new IllegalThreadStateException(
> "Server version response invalid: This could be the result of
> trying to
> connect a non-SSL-enabled client to an SSL-enabled locator."); }
>
>final VersionResponse response = (VersionResponse) readObject;
> serverVersion = response.getVersionOrdinal(); serverVersions.put(address,
> serverVersion); return serverVersion; }catch (EOFException ex) {
>// old locators will not recognize the version request and will close
> the connection }
> ...
> return KnownVersion.OLDEST.ordinal();
>
> The case is when /readObject()/ try to read /VersionResponse/ and then
> throws /EOFException/. As you can see, there is comment in catch
> statement explaining the case, but I'm not sure that I understand it
> correctly. What I assume is that member with old version (less or equal
> to /KnownVersion.OLDEST/) of Geode does not support /VersionRequest/
> message, and will close connection if receive such message. This will
> result with /EOFException/ on remote member that sent /VersionRequest/.
> Is this correct understanding? Is this case still valid with the latest
> non-deprecated version set to /GFE_81/?
>
> The reason why I'm asking this is because in some cases in kuberentes
> cluster, it is possible to get /EOFException/ when remote member is not
> yet accessible. In this case member will still try to send message (e.g.
> /RemoteLocatorJoinRequest/) thinking that it is old member on the other
> side, and that will then result with /ClassCastException/ when reading
> response (e.g. /RemoteLocatorJoinResponse/).
>
> BRs,
>
> Jakov
>
>
Re: Proposal to back port GEODE-8938 to 1.14
Sounds good. Added to 1.14.0 blocker board. On 3/10/21, 8:58 AM, "Darrel Schneider" wrote: +1 From: John Hutchison Sent: Wednesday, March 10, 2021 8:46 AM To: dev@geode.apache.org Subject: Proposal to back port GEODE-8938 to 1.14 Hi all, Proposal to backport GEODE-8938. Rationale: GEODE-8864, which has previously been approved to be backported (and is marked as a release-blocker) relies on GEODE-8938. GEODE-8938 is small, does not alter any feature code, and exists entirely within the compatible-with-Redis area of the code.Additionally, changing GEODE-8864 to not rely on GEODE-8938 would require altering a test. Thanks a lot, John
Re: Proposal to back port GEODE-8938 to 1.14
+1 From: John Hutchison Sent: Wednesday, March 10, 2021 8:46 AM To: dev@geode.apache.org Subject: Proposal to back port GEODE-8938 to 1.14 Hi all, Proposal to backport GEODE-8938. Rationale: GEODE-8864, which has previously been approved to be backported (and is marked as a release-blocker) relies on GEODE-8938. GEODE-8938 is small, does not alter any feature code, and exists entirely within the compatible-with-Redis area of the code.Additionally, changing GEODE-8864 to not rely on GEODE-8938 would require altering a test. Thanks a lot, John
Proposal to back port GEODE-8938 to 1.14
Hi all, Proposal to backport GEODE-8938. Rationale: GEODE-8864, which has previously been approved to be backported (and is marked as a release-blocker) relies on GEODE-8938. GEODE-8938 is small, does not alter any feature code, and exists entirely within the compatible-with-Redis area of the code.Additionally, changing GEODE-8864 to not rely on GEODE-8938 would require altering a test. Thanks a lot, John
