Copilot commented on code in PR #16450:
URL: https://github.com/apache/pinot/pull/16450#discussion_r2255060971
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -245,6 +246,30 @@ public static class Instance {
@Deprecated(since = "1.5.0", forRemoval = true)
public static final String CONFIG_OF_ZOOKEEPR_SERVER = "pinot.zk.server";
Review Comment:
The constant name contains a typo: 'ZOOKEEPR' should be 'ZOOKEEPER'.
```suggestion
public static final String CONFIG_OF_ZOOKEEPER_SERVER_DEPRECATED =
"pinot.zk.server";
```
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartZookeeperCommand.java:
##########
@@ -41,9 +41,20 @@ public class StartZookeeperCommand extends
AbstractBaseAdminCommand implements C
@CommandLine.Option(names = {"-zkPort"}, required = false, description =
"Port to start zookeeper server on.")
private int _zkPort = 2181;
- @CommandLine.Option(names = {"-dataDir"}, required = false, description =
"Directory for zookeper data.")
+ @CommandLine.Option(names = {"-dataDir"}, required = false, description =
"Directory for zookeeper data.")
Review Comment:
The description contains a typo: 'zookeper' should be 'zookeeper' for
consistency.
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java:
##########
@@ -877,9 +892,9 @@ protected List<ValidDocIdsMetadataInfo>
getValidDocIdsMetadata(String tableNameW
ValidDocIdsType validDocIdsType)
throws Exception {
- StringBuilder urlBuilder = new StringBuilder(
+ String responseString = sendGetRequest(
_controllerRequestURLBuilder.forValidDocIdsMetadata(tableNameWithType,
validDocIdsType.toString()));
- String responseString = sendGetRequest(urlBuilder.toString());
- return JsonUtils.stringToObject(responseString, new TypeReference<>() { });
+ return JsonUtils.stringToObject(responseString, new TypeReference<>() {
Review Comment:
[nitpick] The refactoring from StringBuilder to direct method call is good,
but the method chain could be simplified by inlining the variable assignment.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ZkStarter.java:
##########
@@ -154,15 +269,99 @@ public static String getDefaultZkStr() {
*/
public static ZookeeperInstance startLocalZkServer(final int port) {
return startLocalZkServer(port,
- org.apache.commons.io.FileUtils.getTempDirectoryPath() +
File.separator + "test-" + System.currentTimeMillis());
+ org.apache.commons.io.FileUtils.getTempDirectoryPath() +
File.separator + "test-" + System.currentTimeMillis(),
+ null);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL() {
+
+ return
startLocalZkServerWithSSL(NetUtils.findOpenPort(DEFAULT_ZK_TEST_PORT));
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @param port The port to listen on
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL(final int port) {
+ // Create temporary directories for SSL certificates
+ File tempDir = new File(FileUtils.getTempDirectory(), "zkData" +
System.currentTimeMillis());
+ tempDir.mkdirs();
+ String keystorePath = new File(tempDir, "keystore.jks").getAbsolutePath();
+ String truststorePath = new File(tempDir,
"truststore.jks").getAbsolutePath();
+ String password = "testpassword";
+ SSLConfig sslConfig = createTestSSLConfig(keystorePath, truststorePath,
password);
+ return startLocalZkServerWithSSL(port, sslConfig);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @param sslConfig SSL configuration containing keystore and truststore
paths
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL(SSLConfig
sslConfig) {
+ return
startLocalZkServerWithSSL(NetUtils.findOpenPort(DEFAULT_ZK_TEST_PORT),
sslConfig);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled on a specific port
+ * @param port The port to listen on
+ * @param sslConfig SSL configuration containing keystore and truststore
paths
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL(final int port,
SSLConfig sslConfig) {
+ return startLocalZkServerWithSSL(port,
+ org.apache.commons.io.FileUtils.getTempDirectoryPath() +
File.separator + "test-" + System.currentTimeMillis(),
+ sslConfig);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @param port The port to listen on
+ * @param dataDirPath The path for the Zk data directory
+ * @param sslConfig SSL configuration containing keystore and truststore
paths
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public synchronized static ZookeeperInstance startLocalZkServerWithSSL(final
int port, final String dataDirPath,
+ SSLConfig sslConfig) {
+ // Configure ZooKeeper SSL system properties for server
+ System.setProperty("zookeeper.serverCnxnFactory",
"org.apache.zookeeper.server.NettyServerCnxnFactory");
+ System.setProperty("zookeeper.ssl.keyStore.location",
sslConfig.getKeyStorePath());
+ System.setProperty("zookeeper.ssl.keyStore.password",
sslConfig.getKeyStorePassword());
+ System.setProperty("zookeeper.ssl.keyStore.type", "JKS");
+ System.setProperty("zookeeper.ssl.trustStore.location",
sslConfig.getTrustStorePath());
+ System.setProperty("zookeeper.ssl.trustStore.password",
sslConfig.getTrustStorePassword());
+ System.setProperty("zookeeper.ssl.trustStore.type", "JKS");
+
+ // Configure secure port for SSL ZooKeeper
+ System.setProperty("zookeeper.secureClientPort", String.valueOf(port));
+ System.setProperty("zookeeper.secureClientPortAddress", "0.0.0.0");
+
+ // Enable SSL for server
+ System.setProperty("zookeeper.sslQuorum", "true");
+ System.setProperty("zookeeper.ssl.quorum.keyStore.location",
sslConfig.getKeyStorePath());
+ System.setProperty("zookeeper.ssl.quorum.keyStore.password",
sslConfig.getKeyStorePassword());
+ System.setProperty("zookeeper.ssl.quorum.trustStore.location",
sslConfig.getTrustStorePath());
+ System.setProperty("zookeeper.ssl.quorum.trustStore.password",
sslConfig.getTrustStorePassword());
+
+ // Intentionally set clientPort to -1 to disable the regular (non-SSL)
client port for SSL-only mode.
+ // This is not an error value; it is required to prevent ZooKeeper from
listening on the non-SSL port.
+ System.setProperty("zookeeper.clientPort", "-1");
Review Comment:
The comment explains the intentional use of -1 for clientPort, but this
magic number should be defined as a constant to improve maintainability and
make the intent clearer in the code.
```suggestion
// Intentionally set clientPort to DISABLED_CLIENT_PORT to disable the
regular (non-SSL) client port for SSL-only mode.
// This is not an error value; it is required to prevent ZooKeeper from
listening on the non-SSL port.
System.setProperty("zookeeper.clientPort", DISABLED_CLIENT_PORT);
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
