[GitHub] [geode-benchmarks] Bill commented on a change in pull request #137: Adds a router to SNI proxy topology.

[GitBox <gi...@apache.org>]

Bill commented on a change in pull request #137:
URL: https://github.com/apache/geode-benchmarks/pull/137#discussion_r525521126



##########
File path: geode-benchmarks/src/main/java/org/apache/geode/benchmark/topology/ClientServerTopologyWithSniProxy.java
##########
@@ -84,14 +96,23 @@ public static void configure(final TestConfig config,
       case Manual:
         // expect proxy already configured.
     }
+  }
 
-    before(config, new StartClientSNI(LOCATOR_PORT, SNI_PROXY_PORT), CLIENT);
-
+  protected static void configureAfter(final TestConfig config) {
     after(config, new StopClient(), CLIENT);
 
-    if (Manual != sniProxyImplementation) {
+    if (Manual != getSniProxyImplementation()) {
       after(config, new StopSniProxy(), PROXY);
     }
   }
 
+  private static SniProxyImplementation getSniProxyImplementation() {
+    final String sniProp = System.getProperty(WITH_SNI_PROXY_PROPERTY);

Review comment:
       In general, I feel like we should centralize all `System.getProperty()` calls (in every program, including this one.) Centralizing all those calls and then injecting the results into wherever they are needed makes our software more testable (which makes it more maintainable.)
   
   Is the cat already out of the bag in this repo? Is there anything you can do to begin centralizing the `System.getProperty()` calls?

##########
File path: geode-benchmarks/src/main/java/org/apache/geode/benchmark/topology/ClientServerTopologyWithSniProxy.java
##########
@@ -58,23 +64,29 @@ public static SniProxyImplementation valueOfIgnoreCase(String name) {
     }
   }
 
-  public static void configure(final TestConfig config,
-      final SniProxyImplementation sniProxyImplementation) {
+  public static void configure(final TestConfig config) {
     role(config, LOCATOR, NUM_LOCATORS);
     role(config, SERVER, NUM_SERVERS);
     role(config, CLIENT, NUM_CLIENTS);
     role(config, PROXY, NUM_PROXIES);
 
+    configureBefore(config);
+
+    before(config, new StartClientWithSniProxy(LOCATOR_PORT, SNI_PROXY_PORT, PROXY), CLIENT);

Review comment:
       why isn't this `before` config specified in the `configureBefore()` method? Doing so would result in symmetry with `configureAfter()` which is stopping the client and the SNI proxy.

##########
File path: geode-benchmarks/src/test/java/org/apache/geode/benchmark/tests/PartitionedFunctionExecutionBenchmarkTest.java
##########
@@ -15,24 +15,16 @@
 package org.apache.geode.benchmark.tests;
 
 import java.io.File;
-import java.nio.file.Path;
 
-import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.extension.ExtendWith;
-import org.junitpioneer.jupiter.TempDirectory;
+import org.junit.jupiter.api.io.TempDir;
 
 import org.apache.geode.benchmark.LongRange;
 import org.apache.geode.perftest.TestRunners;
 
-@ExtendWith(TempDirectory.class)
 public class PartitionedFunctionExecutionBenchmarkTest {
 
-  private File folder;
-
-  @BeforeEach
-  void createTemporaryFolder(@TempDirectory.TempDir Path tempFolder) {
-    folder = tempFolder.toFile();
-  }
+  @TempDir
+  File folder;

Review comment:
       Nice change!

##########
File path: gradle/dependency-versions.properties
##########
@@ -14,10 +14,10 @@
 # limitations under the License.
 
 # Dependency versions
-junit-jupiter-api.version = 5.3.2
-junit-jupiter-engine.version = 5.3.2
-junit-pioneer.version = 0.3.0
-geode-core.version = 1.8.0
+junit-jupiter-api.version = 5.7.0
+junit-jupiter-engine.version = 5.7.0
+junit-pioneer.version = 1.0.0
+geode-core.version = 1.13.0

Review comment:
       progress ✓

##########
File path: geode-benchmarks/src/test/java/org/apache/geode/benchmark/topology/ClientServerTopologyWithSniProxyTest.java
##########
@@ -44,9 +46,11 @@ public void afterEach() {
 
   @ParameterizedTest
   @EnumSource(SniProxyImplementation.class)
+  @ClearSystemProperty(key = WITH_SNI_PROXY_PROPERTY)
   public void configWithNoSsl(final SniProxyImplementation sniProxyImplementation) {
-    TestConfig testConfig = new TestConfig();
-    ClientServerTopologyWithSNIProxy.configure(testConfig, sniProxyImplementation);
+    System.setProperty(WITH_SNI_PROXY_PROPERTY, sniProxyImplementation.name());

Review comment:
       We shouldn't be "tunneling" parameters this way. We should only be accessing system properties in some sort of main or launcher which then sends them either individually or as part of config structures down to the other software that needs the values.




----------------------------------------------------------------
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:
users@infra.apache.org