[GitHub] [geode] lgtm-com[bot] commented on pull request #5390: ClassLoader isolation
lgtm-com[bot] commented on pull request #5390: URL: https://github.com/apache/geode/pull/5390#issuecomment-675809133 This pull request **introduces 2 alerts** and **fixes 2** when merging a0fc2c1fbd591e060860cd2f0dbb442e53856cc0 into be9a2329d1e06f1ae67baaaf875b6ff20b2922cf - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-b7aa7b67a6e02d8fc2c3eccfffee25d54469b532) **new alerts:** * 2 for Potential input resource leak **fixed alerts:** * 2 for Unused variable, import, function or class 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a change in pull request #5462: GEODE-8435: restore ability to connect gfsh by serialization version
onichols-pivotal commented on a change in pull request #5462:
URL: https://github.com/apache/geode/pull/5462#discussion_r472588532
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
##
@@ -186,6 +186,16 @@ public ResultModel connect(
gfsh.logInfo("failed to get the the remote version.", ex);
}
+// fallback: see if serialization versions matches (Geode 1.12 or later
cluster)
+try {
+ String gfshGeodeSerializationVersion =
gfsh.getGeodeSerializationVersion();
+ String remoteGeodeSerializationVersion =
invoker.getRemoteGeodeSerializationVersion();
+ if
(gfshGeodeSerializationVersion.equals(remoteGeodeSerializationVersion)) {
+return result;
+ }
+} catch (Exception ignore) {
+}
+
Review comment:
I've moved the logic for determining compatible clusters into a central
method to decouple from the network calls needed to get the necessary info.
It's a lot easier now to test just this method in isolation.
Rather than add even further assumptions about product name (even if it were
available from the remote cluster) I've constrained the logic that uses product
version to the narrowest possible scenario (specific known versions and only
used if the remote cluster does not report serialization version)
##
File path:
geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandTest.java
##
@@ -417,4 +417,21 @@ public void connectToManagerBefore1_10() {
.statusIsError()
.containsOutput("Cannot use a 1.14 gfsh client to connect to a 1.9
cluster");
}
+
+ @Test
+ public void connectToManagerBySerializationVersion() {
+when(gfsh.getVersion()).thenReturn("0.0.0");
+when(gfsh.getGeodeSerializationVersion()).thenReturn("1.14.0");
+when(operationInvoker.getRemoteVersion()).thenReturn("0.0.0");
+
when(operationInvoker.getRemoteGeodeSerializationVersion()).thenReturn("1.14.0");
+when(operationInvoker.isConnected()).thenReturn(true);
+
+ResultModel resultModel = new ResultModel();
+when(connectCommand.jmxConnect(any(), anyBoolean(), any(), any(),
anyBoolean()))
+.thenReturn(resultModel);
+
+gfshParserRule.executeAndAssertThat(connectCommand, "connect
--locator=localhost:4040")
+.statusIsSuccess()
+.doesNotContainOutput("Cannot use a 0.0.0 gfsh client to connect to a
0.0.0 cluster");
+ }
Review comment:
The different-minor-version test was a holdover from the old days when
gfsh was locked to same major&minor as the cluster. I removed it. I think
major versions still play a role, as we need to carefully consider how far into
the future we are willing to guarantee forward compatibility. It seems safest
for now to presume that gfsh 1.x won't be interoperable with a future Geode
2.x. The reverse might be more plausible but we can unlock backward major
version compatibility if and when the time comes.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5390: ClassLoader isolation
lgtm-com[bot] commented on pull request #5390: URL: https://github.com/apache/geode/pull/5390#issuecomment-675785863 This pull request **introduces 2 alerts** and **fixes 2** when merging e553fab12c998b87ddd863a361ac9e9ca689e2ba into be9a2329d1e06f1ae67baaaf875b6ff20b2922cf - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-fe2e6cd8864f2169b9bb6c9665319dd121515f2d) **new alerts:** * 2 for Potential input resource leak **fixed alerts:** * 2 for Unused variable, import, function or class 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #5465: GEODE-8419: SSL/TLS protocol and cipher suite configuration is ignored
pivotal-jbarrett commented on a change in pull request #5465:
URL: https://github.com/apache/geode/pull/5465#discussion_r472543514
##
File path:
geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java
##
@@ -98,6 +106,65 @@ private void testBindExceptionMessageFormatting(InetAddress
inetAddress) throws
}
}
+ @Test
+ public void configureSSLEngine() {
+SSLConfig config = new
SSLConfig.Builder().setCiphers("someCipher").setEnabled(true)
+
.setProtocols("someProtocol").setRequireAuth(true).setKeystore("someKeystore.jks")
+.setAlias("someAlias").setTruststore("someTruststore.jks")
+.setEndpointIdentificationEnabled(true).build();
+SSLContext context = mock(SSLContext.class);
+SSLParameters parameters = mock(SSLParameters.class);
+
+SocketCreator socketCreator = new SocketCreator(config, context);
+
+SSLEngine engine = mock(SSLEngine.class);
+when(engine.getSSLParameters()).thenReturn(parameters);
+
+final Object[] setProtocols = new Object[1];
+doAnswer((Answer) invocation -> {
+ setProtocols[0] = invocation.getArgument(0);
+ return null;
+}).when(engine).setEnabledProtocols(any(String[].class));
+
+final Object[] setCiphers = new Object[1];
+doAnswer((Answer) invocation -> {
+ setCiphers[0] = invocation.getArgument(0);
+ return null;
+}).when(engine).setEnabledCipherSuites(any(String[].class));
+
+socketCreator.configureSSLEngine(engine, "somehost", 12345, true);
+
+verify(engine).setUseClientMode(isA(Boolean.class));
Review comment:
Are you familiar with `ArgumentCaptors` in Mokito? I find it to be more
readable approach to the argument validation than `doAnswer` approach.
https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#verify-T-
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] bschuchardt commented on a change in pull request #5465: GEODE-8419: SSL/TLS protocol and cipher suite configuration is ignored
bschuchardt commented on a change in pull request #5465:
URL: https://github.com/apache/geode/pull/5465#discussion_r472539017
##
File path:
geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java
##
@@ -98,6 +102,30 @@ private void testBindExceptionMessageFormatting(InetAddress
inetAddress) throws
}
}
+ @Test
+ public void configureSSLEngine() {
+SSLConfig config = new
SSLConfig.Builder().setCiphers("someCipher").setEnabled(true)
Review comment:
Yes! I'm having lots of "fun" with "any" this week and that's an
excellent idea.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] bschuchardt commented on a change in pull request #5465: GEODE-8419: SSL/TLS protocol and cipher suite configuration is ignored
bschuchardt commented on a change in pull request #5465:
URL: https://github.com/apache/geode/pull/5465#discussion_r472538826
##
File path:
geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java
##
@@ -98,6 +102,30 @@ private void testBindExceptionMessageFormatting(InetAddress
inetAddress) throws
}
}
+ @Test
+ public void configureSSLEngine() {
+SSLConfig config = new
SSLConfig.Builder().setCiphers("someCipher").setEnabled(true)
+
.setProtocols("someProtocol").setRequireAuth(true).setKeystore("someKeystore.jks")
+.setAlias("someAlias").setTruststore("someTruststore.jks")
+.setEndpointIdentificationEnabled(true).build();
+SSLContext context = mock(SSLContext.class);
+SSLParameters parameters = mock(SSLParameters.class);
+
+SocketCreator socketCreator = new SocketCreator(config, context);
+
+SSLEngine engine = mock(SSLEngine.class);
+when(engine.getSSLParameters()).thenReturn(parameters);
+
+engine.setEnableSessionCreation(true);
+socketCreator.configureSSLEngine(engine, "somehost", 12345, true);
+
+verify(engine).setUseClientMode(isA(Boolean.class));
+verify(engine).setSSLParameters(parameters);
+verify(engine).setEnabledCipherSuites(isA(String[].class));
Review comment:
yes, I was being lazy there. I've corrected that
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #5464: GEODE-8432: use regionPath directly instead of getRegion when put eve…
gesterzhou commented on a change in pull request #5464:
URL: https://github.com/apache/geode/pull/5464#discussion_r472530187
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##
@@ -693,15 +693,19 @@ public boolean put(Object object) throws
InterruptedException, CacheException {
boolean isDREvent = isDREvent(sender.getCache(), value);
-Region region = value.getRegion();
-String regionPath = null;
-if (isDREvent) {
- regionPath = region.getFullPath();
-} else {
+String regionPath = value.getRegionPath();
+if (!isDREvent) {
+ Region region = sender.getCache().getRegion(regionPath);
+ if (region == null) {
+if (isDebugEnabled) {
+ logger.debug("The PR " + regionPath + " has not finished
initializing.");
+}
+region = value.getRegion();
Review comment:
After I use getRegion(path, true), I don't need to call
value.getRegion().
Otherwise, value.getRegion() is still necessary. When code arrived here,
it's normal enqueue (not syncWith enqueue), the event.getRegion() will never be
null.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #5464: GEODE-8432: use regionPath directly instead of getRegion when put eve…
gesterzhou commented on a change in pull request #5464:
URL: https://github.com/apache/geode/pull/5464#discussion_r472529105
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##
@@ -693,15 +693,19 @@ public boolean put(Object object) throws
InterruptedException, CacheException {
boolean isDREvent = isDREvent(sender.getCache(), value);
-Region region = value.getRegion();
-String regionPath = null;
-if (isDREvent) {
- regionPath = region.getFullPath();
-} else {
+String regionPath = value.getRegionPath();
+if (!isDREvent) {
+ Region region = sender.getCache().getRegion(regionPath);
Review comment:
I will change to sender.getCache().getRegion(regionPath,true).
It will be null. In the test, the region is destroyed. But we still need to
get the root region. The event will be ignored later by if
(!this.userRegionNameToShadowPRMap.containsKey(regionPath)) {
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634: URL: https://github.com/apache/geode-native/pull/634#discussion_r472527125 ## File path: clicache/src/PoolFactory.hpp ## @@ -279,6 +279,20 @@ namespace Apache /// PoolFactory^ AddServer(String^ host, Int32 port); +/// +/// Set proxy info for SNI connection. +/// +/// +/// Used for connecting via SNI proxy. +/// +/// +/// host the host name or ip address that the server is listening on. +/// +/// +/// port the port that the server is listening on. +/// +PoolFactory^ SetSniProxy(String^ hostname, Int32 port); Review comment: A difference in API is a pretty big surprise to some. Especially when you consider that .NET does expose a Socket concept in the SDK similar to Java. Having a discussion beforehand as to why one API would deviate from another established in an RFC helps reviewers understand the rational for the deviation. I can only make the assumption that the deviation is a result of the C++ layer not wanting to leak `ACE::Socket` outside the internals. There are alternative ways to do that such that you have an API similar to the Java API that is extensible for future proxy implementations or parameters to the proxy connections itself without exposing the internals of the socket implementation. By not having this conversation we have implemented something that solves today's issue but will need to be deprecated or augmented to support future changes to the proxy strategy. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472525063
##
File path: clicache/integration-test2/SNITests.cs
##
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Diagnostics;
+using System.IO;
+using Xunit;
+using PdxTests;
+using System.Collections;
+using System.Collections.Generic;
+using Xunit.Abstractions;
+
+namespace Apache.Geode.Client.IntegrationTests
+{
+[Trait("Category", "Integration")]
+public class SNITests : TestBase, IDisposable
+{
+string currentWorkingDirectory;
+Process dockerProcess;
+private readonly Cache cache_;
+
+public SNITests(ITestOutputHelper testOutputHelper) :
base(testOutputHelper)
+{
+currentWorkingDirectory = Directory.GetCurrentDirectory();
+var clientTruststore = Config.SslClientKeyPath +
@"/truststore_sni.pem";
+
+
+
+var cacheFactory = new CacheFactory();
+cacheFactory.Set("log-level", "none");
+cacheFactory.Set("log-file", "c:/temp/SNITest-csharp.log");
+cacheFactory.Set("ssl-enabled", "true");
+cacheFactory.Set("ssl-truststore", clientTruststore);
+
+cache_ = cacheFactory.Create();
+
+var dc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "-f " +
Config.SniConfigPath + "/docker-compose.yml" + " up -d");
+dc.WaitForExit();
+
+var d = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "exec -t geode gfsh run
--file=/geode/scripts/geode-starter.gfsh");
+d.WaitForExit();
+}
+
+public void Dispose()
+{
+
+var dockerComposeProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "stop");
+dockerComposeProc.WaitForExit();
+
+var dockerProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "container prune -f");
+dockerProc.WaitForExit();
+
+}
+
+private string RunDockerCommand(string dockerCommand)
+{
+ProcessStartInfo startInfo = new ProcessStartInfo();
+startInfo.RedirectStandardOutput = true;
+startInfo.UseShellExecute = false;
+startInfo.FileName = @"C:\Program
Files\Docker\Docker\resources\bin\docker.exe";
+startInfo.Arguments = dockerCommand;
+dockerProcess = Process.Start(startInfo);
+String rVal = dockerProcess.StandardOutput.ReadToEnd();
+dockerProcess.WaitForExit();
+return rVal;
+}
+
+private int ParseProxyPort(string proxyString)
+{
+int colonPosition = proxyString.IndexOf(":");
+string portNumberString = proxyString.Substring(colonPosition + 1);
+return Int32.Parse(portNumberString);
+}
+
+[Fact (Skip = "Docker nut supported in Windows CI")]
+public void ConnectViaProxy()
+{
+var portString = RunDockerCommand("port haproxy");
+var portNumber = ParseProxyPort(portString);
+
+cache_.GetPoolManager()
+.CreateFactory()
+.SetSniProxy("localhost", portNumber)
+.AddLocator("locator-maeve", 10334)
+.Create("pool");
+
+var region = cache_.CreateRegionFactory(RegionShortcut.PROXY)
+ .SetPoolName("pool")
+ .Create("jellyfish");
+
+region.Put("1", "one");
+var value = region.Get("1");
+
+Assert.Equal("one", value);
+cache_.Close();
+}
+
+[Fact (Skip = "Docker nut supported in Windows CI")]
Review comment:
I am running Docker on Windows in GCP just fine. Can I help?
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.
For queries about this service, plea
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472524195
##
File path: clicache/integration-test2/SNITests.cs
##
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Diagnostics;
+using System.IO;
+using Xunit;
+using PdxTests;
+using System.Collections;
+using System.Collections.Generic;
+using Xunit.Abstractions;
+
+namespace Apache.Geode.Client.IntegrationTests
+{
+[Trait("Category", "Integration")]
+public class SNITests : TestBase, IDisposable
+{
+string currentWorkingDirectory;
+Process dockerProcess;
+private readonly Cache cache_;
+
+public SNITests(ITestOutputHelper testOutputHelper) :
base(testOutputHelper)
+{
+currentWorkingDirectory = Directory.GetCurrentDirectory();
+var clientTruststore = Config.SslClientKeyPath +
@"/truststore_sni.pem";
+
+
+
+var cacheFactory = new CacheFactory();
+cacheFactory.Set("log-level", "none");
+cacheFactory.Set("log-file", "c:/temp/SNITest-csharp.log");
+cacheFactory.Set("ssl-enabled", "true");
+cacheFactory.Set("ssl-truststore", clientTruststore);
+
+cache_ = cacheFactory.Create();
+
+var dc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "-f " +
Config.SniConfigPath + "/docker-compose.yml" + " up -d");
+dc.WaitForExit();
+
+var d = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "exec -t geode gfsh run
--file=/geode/scripts/geode-starter.gfsh");
+d.WaitForExit();
+}
+
+public void Dispose()
+{
+
+var dockerComposeProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "stop");
+dockerComposeProc.WaitForExit();
+
+var dockerProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "container prune -f");
Review comment:
https://github.com/lasote/docker_client
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5390: ClassLoader isolation
lgtm-com[bot] commented on pull request #5390: URL: https://github.com/apache/geode/pull/5390#issuecomment-675747104 This pull request **introduces 2 alerts** and **fixes 2** when merging ea366f928d469226b75789a0da8427d02a2a0fbd into be9a2329d1e06f1ae67baaaf875b6ff20b2922cf - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-d8827a790de1c82dba9cd4d0a6b56bbd48458775) **new alerts:** * 2 for Potential input resource leak **fixed alerts:** * 2 for Unused variable, import, function or class 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #5465: GEODE-8419: SSL/TLS protocol and cipher suite configuration is ignored
pivotal-jbarrett commented on a change in pull request #5465:
URL: https://github.com/apache/geode/pull/5465#discussion_r472521802
##
File path:
geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java
##
@@ -98,6 +102,30 @@ private void testBindExceptionMessageFormatting(InetAddress
inetAddress) throws
}
}
+ @Test
+ public void configureSSLEngine() {
+SSLConfig config = new
SSLConfig.Builder().setCiphers("someCipher").setEnabled(true)
+
.setProtocols("someProtocol").setRequireAuth(true).setKeystore("someKeystore.jks")
+.setAlias("someAlias").setTruststore("someTruststore.jks")
+.setEndpointIdentificationEnabled(true).build();
+SSLContext context = mock(SSLContext.class);
+SSLParameters parameters = mock(SSLParameters.class);
+
+SocketCreator socketCreator = new SocketCreator(config, context);
+
+SSLEngine engine = mock(SSLEngine.class);
+when(engine.getSSLParameters()).thenReturn(parameters);
+
+engine.setEnableSessionCreation(true);
+socketCreator.configureSSLEngine(engine, "somehost", 12345, true);
+
+verify(engine).setUseClientMode(isA(Boolean.class));
+verify(engine).setSSLParameters(parameters);
+verify(engine).setEnabledCipherSuites(isA(String[].class));
Review comment:
Should we verify that it is called with a `String[]` only containing the
`"someCipher"`?
##
File path:
geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java
##
@@ -98,6 +102,30 @@ private void testBindExceptionMessageFormatting(InetAddress
inetAddress) throws
}
}
+ @Test
+ public void configureSSLEngine() {
+SSLConfig config = new
SSLConfig.Builder().setCiphers("someCipher").setEnabled(true)
Review comment:
What about a test for `"any"`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] pivotal-eshu commented on a change in pull request #5464: GEODE-8432: use regionPath directly instead of getRegion when put eve…
pivotal-eshu commented on a change in pull request #5464:
URL: https://github.com/apache/geode/pull/5464#discussion_r472499344
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##
@@ -693,15 +693,19 @@ public boolean put(Object object) throws
InterruptedException, CacheException {
boolean isDREvent = isDREvent(sender.getCache(), value);
-Region region = value.getRegion();
-String regionPath = null;
-if (isDREvent) {
- regionPath = region.getFullPath();
-} else {
+String regionPath = value.getRegionPath();
+if (!isDREvent) {
+ Region region = sender.getCache().getRegion(regionPath);
Review comment:
sender.getCache().getRegion(regionPath) should not be null due to
initialization as it invokes region.waitOnInitialization(). If region is null,
I think it indicates region is destroyed.
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##
@@ -693,15 +693,19 @@ public boolean put(Object object) throws
InterruptedException, CacheException {
boolean isDREvent = isDREvent(sender.getCache(), value);
-Region region = value.getRegion();
-String regionPath = null;
-if (isDREvent) {
- regionPath = region.getFullPath();
-} else {
+String regionPath = value.getRegionPath();
+if (!isDREvent) {
+ Region region = sender.getCache().getRegion(regionPath);
+ if (region == null) {
+if (isDebugEnabled) {
+ logger.debug("The PR " + regionPath + " has not finished
initializing.");
+}
+region = value.getRegion();
Review comment:
Should not use value.getRegion() here as it will still cause the hang.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
echobravopapa commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472483145
##
File path: cppcache/include/geode/PoolFactory.hpp
##
@@ -426,6 +426,11 @@ class APACHE_GEODE_EXPORT PoolFactory {
*/
PoolFactory& addServer(const std::string& host, int port);
+ /**
+ * Set proxy info for SNI connection. Used for connecting via SNI proxy.
+ */
+ PoolFactory& setSniProxy(const std::string& hostname, const int port);
Review comment:
""
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
echobravopapa commented on a change in pull request #634: URL: https://github.com/apache/geode-native/pull/634#discussion_r472483043 ## File path: clicache/src/PoolFactory.hpp ## @@ -279,6 +279,20 @@ namespace Apache /// PoolFactory^ AddServer(String^ host, Int32 port); +/// +/// Set proxy info for SNI connection. +/// +/// +/// Used for connecting via SNI proxy. +/// +/// +/// host the host name or ip address that the server is listening on. +/// +/// +/// port the port that the server is listening on. +/// +PoolFactory^ SetSniProxy(String^ hostname, Int32 port); Review comment: Native impl of SNI is consistent with Native's libraries... Java client impl of SNI simply doesn't map to this API. Our understanding is that the light-weight RFC process exists to avoid issues post implementation etc or any surprises... Native's API for SNI does differ from the Java client, but the core feature etc is in line. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pdxcodemonkey commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472481911
##
File path: cppcache/src/ThinClientLocatorHelper.hpp
##
@@ -71,6 +74,8 @@ class ThinClientLocatorHelper {
const ThinClientPoolDM* m_poolDM;
ThinClientLocatorHelper(const ThinClientLocatorHelper&);
ThinClientLocatorHelper& operator=(const ThinClientLocatorHelper&);
+ std::string m_sniProxyHost;
Review comment:
Changes coming to fix this. We changed the new members to conform to
style guide after rebasing on top of your "cryptoimpl removal" change.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pdxcodemonkey commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472481270
##
File path: cppcache/src/PoolAttributes.cpp
##
@@ -44,46 +44,13 @@ PoolAttributes::PoolAttributes()
m_subsEnabled(PoolFactory::DEFAULT_SUBSCRIPTION_ENABLED),
m_multiuserSecurityMode(PoolFactory::DEFAULT_MULTIUSER_SECURE_MODE),
m_isPRSingleHopEnabled(PoolFactory::DEFAULT_PR_SINGLE_HOP_ENABLED),
- m_serverGrp(PoolFactory::DEFAULT_SERVER_GROUP) {}
+ m_serverGrp(PoolFactory::DEFAULT_SERVER_GROUP),
+ m_sniProxyPort(0) {}
+
std::shared_ptr PoolAttributes::clone() {
return std::make_shared(*this);
}
-/** Return true if all the attributes are equal to those of other. */
-bool PoolAttributes::operator==(const PoolAttributes& other) const {
Review comment:
At first I refactored it, per our typical practice, because I was in
modifying things there already. Then I checked to see if it was actually in
use anywhere, and discovered it was not, so out it went.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
echobravopapa commented on a change in pull request #634: URL: https://github.com/apache/geode-native/pull/634#discussion_r472480996 ## File path: cppcache/integration/test/sni-test-config/geode-config/truststore_sni.pem ## @@ -0,0 +1,68 @@ +-BEGIN CERTIFICATE- Review comment: you are probably thinking of the server side... 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
echobravopapa commented on a change in pull request #634: URL: https://github.com/apache/geode-native/pull/634#discussion_r472480140 ## File path: sni-test-config/docker-compose.yml ## @@ -0,0 +1,43 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +version: '3' +services: + geode: +container_name: 'geode' +image: 'apachegeode/geode' +expose: + - '10334' + - '40404' +entrypoint: 'sh' +command: ["-c", "while true; do sleep 600; done"] Review comment: good point, that can be removed 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
echobravopapa commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472479203
##
File path: cppcache/integration/test/SNITest.cpp
##
@@ -108,41 +112,51 @@ class SNITest : public ::testing::Test {
}
std::string certificatePassword;
+ boost::filesystem::path clientSslKeysDir;
boost::filesystem::path currentWorkingDirectory;
+ boost::filesystem::path sniConfigPath;
};
-TEST_F(SNITest, DISABLED_connectViaProxyTest) {
+#if defined(_WIN32)
+TEST_F(SNITest, DISABLE_connectViaProxyTest) {
+#else
+TEST_F(SNITest, connectViaProxyTest) {
+#endif
const auto clientTruststore =
- (currentWorkingDirectory /
- boost::filesystem::path("sni-test-config/geode-config/truststore.jks"));
+ (clientSslKeysDir / boost::filesystem::path("/truststore_sni.pem"));
auto cache = CacheFactory()
- .set("log-level", "DEBUG")
+ .set("log-level", "debug")
+ .set("log-file", "SNITest.log")
.set("ssl-enabled", "true")
.set("ssl-truststore", clientTruststore.string())
.create();
- auto portString = makeItSo("docker port haproxy");
+ auto portString = runDockerCommand("docker port haproxy");
auto portNumber = parseProxyPort(portString);
cache.getPoolManager()
.createFactory()
- .addLocator("localhost", portNumber)
+ .setSniProxy("localhost", portNumber)
+ .addLocator("locator-maeve", 10334)
.create("pool");
auto region = cache.createRegionFactory(RegionShortcut::PROXY)
.setPoolName("pool")
-.create("region");
+.create("jellyfish");
region->put("1", "one");
cache.close();
}
-TEST_F(SNITest, connectionFailsTest) {
+#if defined(_WIN32)
+TEST_F(SNITest, DISABLE_connectWithoutProxyFails) {
Review comment:
aforementioned Docker issues...
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
echobravopapa commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472473631
##
File path: clicache/integration-test2/SNITests.cs
##
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Diagnostics;
+using System.IO;
+using Xunit;
+using PdxTests;
+using System.Collections;
+using System.Collections.Generic;
+using Xunit.Abstractions;
+
+namespace Apache.Geode.Client.IntegrationTests
+{
+[Trait("Category", "Integration")]
+public class SNITests : TestBase, IDisposable
+{
+string currentWorkingDirectory;
+Process dockerProcess;
+private readonly Cache cache_;
+
+public SNITests(ITestOutputHelper testOutputHelper) :
base(testOutputHelper)
+{
+currentWorkingDirectory = Directory.GetCurrentDirectory();
+var clientTruststore = Config.SslClientKeyPath +
@"/truststore_sni.pem";
+
+
+
+var cacheFactory = new CacheFactory();
+cacheFactory.Set("log-level", "none");
+cacheFactory.Set("log-file", "c:/temp/SNITest-csharp.log");
+cacheFactory.Set("ssl-enabled", "true");
+cacheFactory.Set("ssl-truststore", clientTruststore);
+
+cache_ = cacheFactory.Create();
+
+var dc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "-f " +
Config.SniConfigPath + "/docker-compose.yml" + " up -d");
+dc.WaitForExit();
+
+var d = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "exec -t geode gfsh run
--file=/geode/scripts/geode-starter.gfsh");
+d.WaitForExit();
+}
+
+public void Dispose()
+{
+
+var dockerComposeProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "stop");
+dockerComposeProc.WaitForExit();
+
+var dockerProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "container prune -f");
+dockerProc.WaitForExit();
+
+}
+
+private string RunDockerCommand(string dockerCommand)
+{
+ProcessStartInfo startInfo = new ProcessStartInfo();
+startInfo.RedirectStandardOutput = true;
+startInfo.UseShellExecute = false;
+startInfo.FileName = @"C:\Program
Files\Docker\Docker\resources\bin\docker.exe";
+startInfo.Arguments = dockerCommand;
+dockerProcess = Process.Start(startInfo);
+String rVal = dockerProcess.StandardOutput.ReadToEnd();
+dockerProcess.WaitForExit();
+return rVal;
+}
+
+private int ParseProxyPort(string proxyString)
+{
+int colonPosition = proxyString.IndexOf(":");
+string portNumberString = proxyString.Substring(colonPosition + 1);
+return Int32.Parse(portNumberString);
+}
+
+[Fact (Skip = "Docker nut supported in Windows CI")]
+public void ConnectViaProxy()
+{
+var portString = RunDockerCommand("port haproxy");
+var portNumber = ParseProxyPort(portString);
+
+cache_.GetPoolManager()
+.CreateFactory()
+.SetSniProxy("localhost", portNumber)
+.AddLocator("locator-maeve", 10334)
+.Create("pool");
+
+var region = cache_.CreateRegionFactory(RegionShortcut.PROXY)
+ .SetPoolName("pool")
+ .Create("jellyfish");
+
+region.Put("1", "one");
+var value = region.Get("1");
+
+Assert.Equal("one", value);
+cache_.Close();
+}
+
+[Fact (Skip = "Docker nut supported in Windows CI")]
Review comment:
its Docker on GCP that is specifically the issue, but thanks for
catching the typo
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.
For queries
[GitHub] [geode-native] echobravopapa commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
echobravopapa commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472472720
##
File path: clicache/integration-test2/SNITests.cs
##
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Diagnostics;
+using System.IO;
+using Xunit;
+using PdxTests;
+using System.Collections;
+using System.Collections.Generic;
+using Xunit.Abstractions;
+
+namespace Apache.Geode.Client.IntegrationTests
+{
+[Trait("Category", "Integration")]
+public class SNITests : TestBase, IDisposable
+{
+string currentWorkingDirectory;
+Process dockerProcess;
+private readonly Cache cache_;
+
+public SNITests(ITestOutputHelper testOutputHelper) :
base(testOutputHelper)
+{
+currentWorkingDirectory = Directory.GetCurrentDirectory();
+var clientTruststore = Config.SslClientKeyPath +
@"/truststore_sni.pem";
+
+
+
+var cacheFactory = new CacheFactory();
+cacheFactory.Set("log-level", "none");
+cacheFactory.Set("log-file", "c:/temp/SNITest-csharp.log");
+cacheFactory.Set("ssl-enabled", "true");
+cacheFactory.Set("ssl-truststore", clientTruststore);
+
+cache_ = cacheFactory.Create();
+
+var dc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "-f " +
Config.SniConfigPath + "/docker-compose.yml" + " up -d");
+dc.WaitForExit();
+
+var d = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "exec -t geode gfsh run
--file=/geode/scripts/geode-starter.gfsh");
+d.WaitForExit();
+}
+
+public void Dispose()
+{
+
+var dockerComposeProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "stop");
+dockerComposeProc.WaitForExit();
+
+var dockerProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "container prune -f");
Review comment:
not that we have found, unfortunately...
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] smgoller merged pull request #5458: GEODE-8321 - Use Liberica JDK for testing - Part 1.
smgoller merged pull request #5458: URL: https://github.com/apache/geode/pull/5458 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] bschuchardt opened a new pull request #5465: GEODE-8419: SSL/TLS protocol and cipher suite configuration is ignored
bschuchardt opened a new pull request #5465: URL: https://github.com/apache/geode/pull/5465 Configure cipher suites when creating an SSLEngine 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. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] gesterzhou opened a new pull request #5464: GEODE-8432: use regionPath directly instead of getRegion when put eve…
gesterzhou opened a new pull request #5464: URL: https://github.com/apache/geode/pull/5464 …nt into parallelGatewaySenderQueue 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. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #5462: GEODE-8435: restore ability to connect gfsh by serialization version
Bill commented on a change in pull request #5462:
URL: https://github.com/apache/geode/pull/5462#discussion_r472360083
##
File path:
geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandTest.java
##
@@ -417,4 +417,21 @@ public void connectToManagerBefore1_10() {
.statusIsError()
.containsOutput("Cannot use a 1.14 gfsh client to connect to a 1.9
cluster");
}
+
+ @Test
+ public void connectToManagerBySerializationVersion() {
+when(gfsh.getVersion()).thenReturn("0.0.0");
+when(gfsh.getGeodeSerializationVersion()).thenReturn("1.14.0");
+when(operationInvoker.getRemoteVersion()).thenReturn("0.0.0");
+
when(operationInvoker.getRemoteGeodeSerializationVersion()).thenReturn("1.14.0");
+when(operationInvoker.isConnected()).thenReturn(true);
+
+ResultModel resultModel = new ResultModel();
+when(connectCommand.jmxConnect(any(), anyBoolean(), any(), any(),
anyBoolean()))
+.thenReturn(resultModel);
+
+gfshParserRule.executeAndAssertThat(connectCommand, "connect
--locator=localhost:4040")
+.statusIsSuccess()
+.doesNotContainOutput("Cannot use a 0.0.0 gfsh client to connect to a
0.0.0 cluster");
+ }
Review comment:
This is my understanding of the equivalence classes for testing are (rsv
= remote serialization version read via JMX, rmv = remote marketing version
read via JMX)
```
rsv >= 1.12 && rmv == dont-care
compatible, covered by connectToManagerBySerializationVersion()
rsv < 1.12
impossible
rsv unknown
rmv >= 1.10
compatible, covered by connectToManagerOlderThan1_10() sic
rmv < 1.10
incompatible
rmv unknown
incompatible
```
By the way `connectToManagerOlderThan1_10()` is mis-named: it's actually
connecting to version 1.10—not a version _older_ than 1.10. Please fix this
name.
But then I also see these tests:
```
connectToManagerWithDifferentMajorVersion()
connectToManagerWithDifferentMinorVersion()
```
So major and minor versions seem to be important too. But only for certain
version ranges? Actually `ConnectCommand` does not, in general respect semver
at all. I think those tests need to be removed.
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
##
@@ -186,6 +186,16 @@ public ResultModel connect(
gfsh.logInfo("failed to get the the remote version.", ex);
}
+// fallback: see if serialization versions matches (Geode 1.12 or later
cluster)
+try {
+ String gfshGeodeSerializationVersion =
gfsh.getGeodeSerializationVersion();
+ String remoteGeodeSerializationVersion =
invoker.getRemoteGeodeSerializationVersion();
+ if
(gfshGeodeSerializationVersion.equals(remoteGeodeSerializationVersion)) {
+return result;
+ }
+} catch (Exception ignore) {
+}
+
Review comment:
Intuitively, it seems that this logic should be tried first and that the
alternative (examining the marketing version) should be the fallback.
Another issue with this block of code is that the exception is silently
ignored. At a minimum we need a clear comment explaining where the exception
comes from since none of the method signatures inside the block have `throws`
clauses. Seems like we could eliminate this `try-catch` entirely.
Also, it would aid in maintenance if the code on lines 171-197 was extracted
into a method. This would simplify `ConnectCommand.connect()` and would give us
the opportunity to name the new method, hide variables, etc. By the way, the
comment at line 171 lies:
```
// since 1.14, only allow gfsh to connect to cluster that's older than
1.10
```
Should say "newer" not "older".
Um also, the logic on line 178-179:
```
if (versionComponent(remoteVersion, VERSION_MAJOR).equals("1") &&
minorVersion >= 10 ||
versionComponent(remoteVersion, VERSION_MAJOR).equals("9") &&
minorVersion >= 9) {
```
Needs a gating condition that checks the product name so that these version
checks only succeed for Geode and GemFire respectively.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] kirklund merged pull request #5451: GEODE-8425: Add new handling netsearch statistics
kirklund merged pull request #5451: URL: https://github.com/apache/geode/pull/5451 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] jujoramos opened a new pull request #5463: [WIP]: GEM-2931
jujoramos opened a new pull request #5463: URL: https://github.com/apache/geode/pull/5463 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode-native] mkevo merged pull request #629: GEODE-8364: Change log level at runtime
mkevo merged pull request #629: URL: https://github.com/apache/geode-native/pull/629 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode-native] mkevo merged pull request #628: GEODE-8344: Add GatewaySenderEventCallbackArgument class
mkevo merged pull request #628: URL: https://github.com/apache/geode-native/pull/628 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on pull request #628: GEODE-8344: Add GatewaySenderEventCallbackArgument class
alb3rtobr commented on pull request #628: URL: https://github.com/apache/geode-native/pull/628#issuecomment-675311553 thanks for the reviews! Sorry but I need someone to merge the PR because Im not a committer 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
