[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5027: Do not derive host/port from instance id in HelixServerStarter

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

Jackie-Jiang commented on a change in pull request #5027: Do not derive host/port from instance id in HelixServerStarter
URL: https://github.com/apache/incubator-pinot/pull/5027#discussion_r373158026
 
 

 ##########
 File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ServerStarterIntegrationTest.java
 ##########
 @@ -33,119 +29,105 @@
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
-import static org.apache.pinot.common.utils.CommonConstants.Helix.KEY_OF_SERVER_NETTY_HOST;
-import static org.apache.pinot.common.utils.CommonConstants.Helix.KEY_OF_SERVER_NETTY_PORT;
+import static org.apache.pinot.common.utils.CommonConstants.Helix.*;
 import static org.apache.pinot.common.utils.CommonConstants.Server.CONFIG_OF_INSTANCE_ID;
 import static org.testng.Assert.assertEquals;
 
 
 public class ServerStarterIntegrationTest extends ControllerTest {
-  public static final String SERVER1 = "Server1";
+  private static final String CUSTOM_INSTANCE_ID = "CustomInstance";
+  private static final String CUSTOM_HOST = "CustomHost";
+  private static final int CUSTOM_PORT = 10001;
 
   @BeforeClass
-  public void setUp()
-      throws Exception {
+  public void setUp() {
     startZk();
     startController();
   }
 
   @AfterClass
-  public void tearDown()
-      throws Exception {
+  public void tearDown() {
     stopController();
     stopZk();
   }
 
-  private void verifyZkConfigData(HelixServerStarter helixServerStarter, String expectedInstanceName,
-      String expectedHostname, String expectedPort) {
-    // Verify the serverId, host and port are set correctly in Zk.
-    HelixManager helixManager = helixServerStarter.getHelixManager();
-    PropertyKey.Builder keyBuilder = helixManager.getHelixDataAccessor().keyBuilder();
-    InstanceConfig config = helixManager.getHelixDataAccessor().
-        getProperty(keyBuilder.instanceConfig(helixServerStarter.getHelixManager().getInstanceName()));
-    helixServerStarter.stop();
+  private void verifyInstanceConfig(HelixServerStarter helixServerStarter, String expectedInstanceId,
+      String expectedHost, int expectedPort) {
+    assertEquals(helixServerStarter.getInstanceId(), expectedInstanceId);
+
+    InstanceConfig instanceConfig =
+        _helixDataAccessor.getProperty(_helixDataAccessor.keyBuilder().instanceConfig(expectedInstanceId));
+    assertEquals(instanceConfig.getHostName(), expectedHost);
+    assertEquals(Integer.parseInt(instanceConfig.getPort()), expectedPort);
 
-    assertEquals(config.getInstanceName(), expectedInstanceName);
-    // By default (auto joined instances), server instance name is of format: {@code Server_<hostname>_<port>}, e.g.
-    // {@code Server_localhost_12345}, hostname is of format: {@code Server_<hostname>}, e.g. {@code Server_localhost}.
-    // More details refer to the class ServerInstance.
-    assertEquals(config.getHostName(), expectedHostname);
-    assertEquals(config.getPort(), expectedPort);
+    helixServerStarter.stop();
 
 Review comment:
   Make sense, changed

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org