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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
