You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by ash211 <gi...@git.apache.org> on 2017/10/24 08:53:38 UTC
[GitHub] spark pull request #19217: [SPARK-21991][LAUNCHER] LauncherServer acceptConn...
Github user ash211 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19217#discussion_r146493161
--- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
@@ -232,20 +232,20 @@ public void run() {
};
ServerConnection clientConnection = new ServerConnection(client, timeout);
Thread clientThread = factory.newThread(clientConnection);
- synchronized (timeout) {
- clientThread.start();
- synchronized (clients) {
- clients.add(clientConnection);
- }
- long timeoutMs = getConnectionTimeout();
- // 0 is used for testing to avoid issues with clock resolution / thread scheduling,
- // and force an immediate timeout.
- if (timeoutMs > 0) {
- timeoutTimer.schedule(timeout, getConnectionTimeout());
- } else {
- timeout.run();
- }
+ synchronized (clients) {
+ clients.add(clientConnection);
+ }
+
+ long timeoutMs = getConnectionTimeout();
+ // 0 is used for testing to avoid issues with clock resolution / thread scheduling,
+ // and force an immediate timeout.
+ if (timeoutMs > 0) {
+ timeoutTimer.schedule(timeout, timeoutMs);
--- End diff --
+1 on not calling `getConnectionTimeout()` multiple times
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org