[GitHub] [spark] juliuszsompolski commented on a change in pull request #28751: [SPARK-31926][SQL][test-hive1.2] Fix concurrency issue for ThriftCLIService to getPortNumber

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

juliuszsompolski commented on a change in pull request #28751:
URL: https://github.com/apache/spark/pull/28751#discussion_r437262570



##########
File path: sql/hive-thriftserver/v1.2/src/main/java/org/apache/hive/service/cli/thrift/ThriftBinaryCLIService.java
##########
@@ -45,13 +46,13 @@ public ThriftBinaryCLIService(CLIService cliService) {
   }
 
   @Override
-  public void run() {
+  protected void initializeServer() {
     try {
       // Server thread pool
       String threadPoolName = "HiveServer2-Handler-Pool";
       ExecutorService executorService = new ThreadPoolExecutor(minWorkerThreads, maxWorkerThreads,
-          workerKeepAliveTime, TimeUnit.SECONDS, new SynchronousQueue<Runnable>(),
-          new ThreadFactoryWithGarbageCleanup(threadPoolName));
+        workerKeepAliveTime, TimeUnit.SECONDS, new SynchronousQueue<Runnable>(),
+        new ThreadFactoryWithGarbageCleanup(threadPoolName));

Review comment:
       Spark does not have an official styling guide for Java, so I think the previous 4 space indents.
   Could you revert the indent/styling changes in those files, to make tracking changes and merging between branches easier?
   I find it easier to track which code is directly imported from Hive, and which was modified for Spark, if it's not modified with styling changes, so I can diff it directly with Hive files.

##########
File path: sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/SharedThriftServer.scala
##########
@@ -74,18 +86,31 @@ trait SharedThriftServer extends SharedSparkSession {
     // Set the HIVE_SERVER2_THRIFT_PORT to 0, so it could randomly pick any free port to use.
     // It's much more robust than set a random port generated by ourselves ahead
     sqlContext.setConf(ConfVars.HIVE_SERVER2_THRIFT_PORT.varname, "0")
-    hiveServer2 = HiveThriftServer2.startWithContext(sqlContext)
-    hiveServer2.getServices.asScala.foreach {
-      case t: ThriftCLIService if t.getPortNumber != 0 =>
-        serverPort = t.getPortNumber
-        logInfo(s"Started HiveThriftServer2: port=$serverPort, attempt=$attempt")
-      case _ =>
-    }
+    // Set the HIVE_SERVER2_THRIFT_HTTP_PORT to 0, so it could randomly pick any free port to use.
+    // It's much more robust than set a random port generated by ourselves ahead

Review comment:
       nit: this duplicates the comment above. The two comments could be merged.

##########
File path: sql/hive-thriftserver/v1.2/src/main/java/org/apache/hive/service/cli/thrift/ThriftBinaryCLIService.java
##########
@@ -97,14 +98,20 @@ public void run() {
       // TCP Server
       server = new TThreadPoolServer(sargs);
       server.setServerEventHandler(serverEventHandler);
-      String msg = "Starting " + ThriftBinaryCLIService.class.getSimpleName() + " on port "
-          + serverSocket.getServerSocket().getLocalPort() + " with " + minWorkerThreads + "..." + maxWorkerThreads + " worker threads";
+      String msg = "Starting " + getName() + " on port " + portNum + " with " + minWorkerThreads +
+          "..." + maxWorkerThreads + " worker threads";

Review comment:
       It this log message change needed? Does it introduce any actual changes?
   I think they may be consumers waiting and parsing this line to make sure the Thriftserver is running, and parse the port etc, so if the format changes, it may break such matches.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org