You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rxin <gi...@git.apache.org> on 2014/12/05 22:22:09 UTC
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
GitHub user rxin opened a pull request:
https://github.com/apache/spark/pull/3625
[SPARK-4740] [WIP] Create multiple concurrent connections between two peer nodes in Netty.
Need to test & add test cases.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rxin/spark SPARK-4740
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/3625.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3625
----
commit 4f216736024f48ed9fa3fca6ea5953495def8e51
Author: Reynold Xin <rx...@databricks.com>
Date: 2014-12-05T21:20:58Z
[SPARK-4740] Create multiple concurrent connections between two peer nodes in Netty.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-66392255
I'm merging this one in master & branch-1.2.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65869095
[Test build #24198 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24198/consoleFull) for PR 3625 at commit [`9076b4a`](https://github.com/apache/spark/commit/9076b4a0f9e648ae5451795c3d024d491accd6e0).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65886767
[Test build #24207 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24207/consoleFull) for PR 3625 at commit [`ad4241a`](https://github.com/apache/spark/commit/ad4241ad8e6f353e5be872e20f6c4261180c648a).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65883784
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24205/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/3625#discussion_r21406868
--- Diff: network/common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java ---
@@ -143,43 +184,41 @@ public void initChannel(SocketChannel ch) {
assert client != null : "Channel future completed successfully with null client";
// Execute any client bootstraps synchronously before marking the Client as successful.
- long preBootstrap = System.currentTimeMillis();
+ long preBootstrap = System.nanoTime();
logger.debug("Connection to {} successful, running bootstraps...", address);
try {
for (TransportClientBootstrap clientBootstrap : clientBootstraps) {
clientBootstrap.doBootstrap(client);
}
} catch (Exception e) { // catch non-RuntimeExceptions too as bootstrap may be written in Scala
- long bootstrapTime = System.currentTimeMillis() - preBootstrap;
- logger.error("Exception while bootstrapping client after " + bootstrapTime + " ms", e);
+ long bootstrapTimeMs = (System.nanoTime() - preBootstrap) / 1000000;
+ logger.error("Exception while bootstrapping client after " + bootstrapTimeMs + " ms", e);
client.close();
throw Throwables.propagate(e);
}
- long postBootstrap = System.currentTimeMillis();
-
- // Successful connection & bootstrap -- in the event that two threads raced to create a client,
- // use the first one that was put into the connectionPool and close the one we made here.
- TransportClient oldClient = connectionPool.putIfAbsent(address, client);
- if (oldClient == null) {
- logger.debug("Successfully created connection to {} after {} ms ({} ms spent in bootstraps)",
- address, postBootstrap - preConnect, postBootstrap - preBootstrap);
- return client;
- } else {
- logger.debug("Two clients were created concurrently after {} ms, second will be disposed.",
- postBootstrap - preConnect);
- client.close();
- return oldClient;
- }
+ long postBootstrap = System.nanoTime();
+
+ logger.debug("Successfully created connection to {} after {} ms ({} ms spent in bootstraps)",
+ address, (postBootstrap - preConnect) / 1000000, (postBootstrap - preBootstrap) / 1000000);
+
+ return client;
}
/** Close all connections in the connection pool, and shutdown the worker thread pool. */
@Override
public void close() {
- for (TransportClient client : connectionPool.values()) {
- try {
- client.close();
- } catch (RuntimeException e) {
- logger.warn("Ignoring exception during close", e);
+ // Go through all clients and close them if they are active.
+ for (ClientPool clientPool : connectionPool.values()) {
+ for (int i = 0; i < clientPool.clients.length; i++) {
+ TransportClient client = clientPool.clients[i];
+ if (client != null) {
+ clientPool.clients[i] = null;
--- End diff --
need synchronization here too
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65865836
[Test build #24193 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24193/consoleFull) for PR 3625 at commit [`3e1306c`](https://github.com/apache/spark/commit/3e1306cd443fe3f580e44bf947b00223f0beb747).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65865847
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24193/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/3625#discussion_r21406852
--- Diff: network/common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java ---
@@ -143,43 +184,41 @@ public void initChannel(SocketChannel ch) {
assert client != null : "Channel future completed successfully with null client";
// Execute any client bootstraps synchronously before marking the Client as successful.
- long preBootstrap = System.currentTimeMillis();
+ long preBootstrap = System.nanoTime();
logger.debug("Connection to {} successful, running bootstraps...", address);
try {
for (TransportClientBootstrap clientBootstrap : clientBootstraps) {
clientBootstrap.doBootstrap(client);
}
} catch (Exception e) { // catch non-RuntimeExceptions too as bootstrap may be written in Scala
- long bootstrapTime = System.currentTimeMillis() - preBootstrap;
- logger.error("Exception while bootstrapping client after " + bootstrapTime + " ms", e);
+ long bootstrapTimeMs = (System.nanoTime() - preBootstrap) / 1000000;
+ logger.error("Exception while bootstrapping client after " + bootstrapTimeMs + " ms", e);
client.close();
throw Throwables.propagate(e);
}
- long postBootstrap = System.currentTimeMillis();
-
- // Successful connection & bootstrap -- in the event that two threads raced to create a client,
- // use the first one that was put into the connectionPool and close the one we made here.
- TransportClient oldClient = connectionPool.putIfAbsent(address, client);
- if (oldClient == null) {
- logger.debug("Successfully created connection to {} after {} ms ({} ms spent in bootstraps)",
- address, postBootstrap - preConnect, postBootstrap - preBootstrap);
- return client;
- } else {
- logger.debug("Two clients were created concurrently after {} ms, second will be disposed.",
- postBootstrap - preConnect);
- client.close();
- return oldClient;
- }
+ long postBootstrap = System.nanoTime();
+
+ logger.debug("Successfully created connection to {} after {} ms ({} ms spent in bootstraps)",
+ address, (postBootstrap - preConnect) / 1000000, (postBootstrap - preBootstrap) / 1000000);
--- End diff --
2 spaces for indent
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65880366
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24200/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65885553
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24206/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65879741
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24199/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/3625
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/3625#discussion_r21406023
--- Diff: network/common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java ---
@@ -56,12 +57,28 @@
* TransportClient, all given {@link TransportClientBootstrap}s will be run.
*/
public class TransportClientFactory implements Closeable {
+
+ /** A simple data structure to track the pool of clients between two peer nodes. */
+ private class ClientPool {
+ TransportClient[] clients;
+ Object[] locks;
+
+ public ClientPool() {
+ clients = new TransportClient[numConnectionsPerPeer];
--- End diff --
We can make this a private static class if we make this a constructor parameter.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65874403
[Test build #24198 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24198/consoleFull) for PR 3625 at commit [`9076b4a`](https://github.com/apache/spark/commit/9076b4a0f9e648ae5451795c3d024d491accd6e0).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/3625#discussion_r21413733
--- Diff: network/common/src/test/java/org/apache/spark/network/TransportClientFactorySuite.java ---
@@ -57,16 +62,113 @@ public void tearDown() {
JavaUtils.closeQuietly(server2);
}
+ /**
+ * Request a bunch of clients to a single server to test
+ * we create up to maxConnections of clients.
+ */
+ private void testClientReuse(final int maxConnections) throws IOException {
+ TransportConf conf = new TransportConf(new ConfigProvider() {
+ @Override
+ public String get(String name) {
+ if (name.equals("spark.shuffle.io.numConnectionsPerPeer")) {
+ return Integer.toString(maxConnections);
+ } else {
+ throw new NoSuchElementException();
+ }
+ }
+ });
+
+ RpcHandler rpcHandler = new NoOpRpcHandler();
+ TransportContext context = new TransportContext(conf, rpcHandler);
+ TransportClientFactory factory = context.createClientFactory();
+ HashSet<TransportClient> clients = new HashSet<TransportClient>();
+ for (int i = 0; i < maxConnections * 10; i++) {
+ TransportClient client = factory.createClient(TestUtils.getLocalHost(), server1.getPort());
+ assert(client.isActive());
+ clients.add(client);
+ }
+
+ assert(clients.size() == maxConnections);
--- End diff --
seems like we should close the clients afterwards
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/3625#discussion_r21406649
--- Diff: network/common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java ---
@@ -97,23 +116,45 @@ public TransportClient createClient(String remoteHost, int remotePort) throws IO
// Get connection from the connection pool first.
// If it is not found or not active, create a new one.
final InetSocketAddress address = new InetSocketAddress(remoteHost, remotePort);
- TransportClient cachedClient = connectionPool.get(address);
+
+ // Create the ClientPool if we don't have it yet.
+ ClientPool clientPool = connectionPool.get(address);
+ if (clientPool == null) {
+ clientPool = connectionPool.putIfAbsent(address, new ClientPool());
--- End diff --
putIfAbsent returns the *previous* value, so this would be null
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65885551
[Test build #24206 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24206/consoleFull) for PR 3625 at commit [`ad4241a`](https://github.com/apache/spark/commit/ad4241ad8e6f353e5be872e20f6c4261180c648a).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65874408
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24198/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65874329
[Test build #24199 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24199/consoleFull) for PR 3625 at commit [`41dfcb2`](https://github.com/apache/spark/commit/41dfcb260e71ecc5d497d64c80f976f350f59be5).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65880355
[Test build #24200 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24200/consoleFull) for PR 3625 at commit [`0fefabb`](https://github.com/apache/spark/commit/0fefabb9d63dec4ad2175338ee7d42ad622a9536).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65869325
Looks mostly good to me, a few remaining synchronization issues. Will take another long look after you address all comments. I'd really appreciate a test, though, if we can get one in -- we really don't want to be regressing at this point, and we also really want to make sure we're fixing the issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65884482
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65883623
[Test build #24206 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24206/consoleFull) for PR 3625 at commit [`ad4241a`](https://github.com/apache/spark/commit/ad4241ad8e6f353e5be872e20f6c4261180c648a).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/3625#discussion_r21406760
--- Diff: network/common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java ---
@@ -97,23 +116,45 @@ public TransportClient createClient(String remoteHost, int remotePort) throws IO
// Get connection from the connection pool first.
// If it is not found or not active, create a new one.
final InetSocketAddress address = new InetSocketAddress(remoteHost, remotePort);
- TransportClient cachedClient = connectionPool.get(address);
+
+ // Create the ClientPool if we don't have it yet.
+ ClientPool clientPool = connectionPool.get(address);
+ if (clientPool == null) {
+ clientPool = connectionPool.putIfAbsent(address, new ClientPool());
+ }
+
+ int clientIndex = rand.nextInt(numConnectionsPerPeer);
+ TransportClient cachedClient = clientPool.clients[clientIndex];
if (cachedClient != null) {
if (cachedClient.isActive()) {
logger.trace("Returning cached connection to {}: {}", address, cachedClient);
return cachedClient;
} else {
logger.info("Found inactive connection to {}, closing it.", address);
- connectionPool.remove(address, cachedClient); // Remove inactive clients.
+ clientPool.clients[clientIndex] = null; // Remove inactive clients.
--- End diff --
Shouldn't this be behind a lock?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] [WIP] Create multiple concurrent ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65857520
[Test build #24193 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24193/consoleFull) for PR 3625 at commit [`3e1306c`](https://github.com/apache/spark/commit/3e1306cd443fe3f580e44bf947b00223f0beb747).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65886768
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24207/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65879737
[Test build #24199 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24199/consoleFull) for PR 3625 at commit [`41dfcb2`](https://github.com/apache/spark/commit/41dfcb260e71ecc5d497d64c80f976f350f59be5).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/3625#discussion_r21413717
--- Diff: network/common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java ---
@@ -143,43 +190,41 @@ public void initChannel(SocketChannel ch) {
assert client != null : "Channel future completed successfully with null client";
// Execute any client bootstraps synchronously before marking the Client as successful.
- long preBootstrap = System.currentTimeMillis();
+ long preBootstrap = System.nanoTime();
logger.debug("Connection to {} successful, running bootstraps...", address);
try {
for (TransportClientBootstrap clientBootstrap : clientBootstraps) {
clientBootstrap.doBootstrap(client);
}
} catch (Exception e) { // catch non-RuntimeExceptions too as bootstrap may be written in Scala
- long bootstrapTime = System.currentTimeMillis() - preBootstrap;
- logger.error("Exception while bootstrapping client after " + bootstrapTime + " ms", e);
+ long bootstrapTimeMs = (System.nanoTime() - preBootstrap) / 1000000;
+ logger.error("Exception while bootstrapping client after " + bootstrapTimeMs + " ms", e);
client.close();
throw Throwables.propagate(e);
}
- long postBootstrap = System.currentTimeMillis();
-
- // Successful connection & bootstrap -- in the event that two threads raced to create a client,
- // use the first one that was put into the connectionPool and close the one we made here.
- TransportClient oldClient = connectionPool.putIfAbsent(address, client);
- if (oldClient == null) {
- logger.debug("Successfully created connection to {} after {} ms ({} ms spent in bootstraps)",
- address, postBootstrap - preConnect, postBootstrap - preBootstrap);
- return client;
- } else {
- logger.debug("Two clients were created concurrently after {} ms, second will be disposed.",
- postBootstrap - preConnect);
- client.close();
- return oldClient;
- }
+ long postBootstrap = System.nanoTime();
+
+ logger.debug("Successfully created connection to {} after {} ms ({} ms spent in bootstraps)",
+ address, (postBootstrap - preConnect) / 1000000, (postBootstrap - preBootstrap) / 1000000);
+
+ return client;
}
/** Close all connections in the connection pool, and shutdown the worker thread pool. */
@Override
public void close() {
- for (TransportClient client : connectionPool.values()) {
- try {
- client.close();
- } catch (RuntimeException e) {
- logger.warn("Ignoring exception during close", e);
+ // Go through all clients and close them if they are active.
+ for (ClientPool clientPool : connectionPool.values()) {
+ for (int i = 0; i < clientPool.clients.length; i++) {
+ TransportClient client = clientPool.clients[i];
+ if (client != null) {
+ clientPool.clients[i] = null;
+ try {
+ client.close();
--- End diff --
Q: Can we use JavaUtils.closeQuietly(client) here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65885123
[Test build #24207 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24207/consoleFull) for PR 3625 at commit [`ad4241a`](https://github.com/apache/spark/commit/ad4241ad8e6f353e5be872e20f6c4261180c648a).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65885045
Jenkins, retest this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4740] Create multiple concurrent connec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3625#issuecomment-65875429
[Test build #24200 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24200/consoleFull) for PR 3625 at commit [`0fefabb`](https://github.com/apache/spark/commit/0fefabb9d63dec4ad2175338ee7d42ad622a9536).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org