[GitHub] storm pull request #2669: [STORM-3055] remove conext connection cache

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm/pull/2669


---

[GitHub] storm pull request #2669: [STORM-3055] remove conext connection cache

[pczb]

Github user pczb commented on a diff in the pull request:

https://github.com/apache/storm/pull/2669#discussion_r18812
  
--- Diff: storm-client/src/jvm/org/apache/storm/messaging/IContext.java ---
@@ -38,6 +38,7 @@
 
 /**
  * This method establishes a server side connection
+ * implementation should return a new connection every call
--- End diff --

remove bind specification,  since the new and old netty context never cache 
server connections, server connections is add to array or map just for cleanup 


---

[GitHub] storm pull request #2669: [STORM-3055] remove conext connection cache

[srdo]

Github user srdo commented on a diff in the pull request:

https://github.com/apache/storm/pull/2669#discussion_r188377925
  
--- Diff: storm-client/src/jvm/org/apache/storm/messaging/IContext.java ---
@@ -38,6 +38,7 @@
 
 /**
  * This method establishes a server side connection
+ * implementation should return a new connection every call
--- End diff --

Bind doesn't always return a new connection, the local Context will return 
a cached one if possible. Consider changing to something like "This method 
returns a server side connection. If one does not exist for the given ID and 
port, a new one will be established."


---

[GitHub] storm pull request #2669: [STORM-3055] remove conext connection cache

[pczb]

Github user pczb commented on a diff in the pull request:

https://github.com/apache/storm/pull/2669#discussion_r188355473
  
--- Diff: storm-client/src/jvm/org/apache/storm/messaging/netty/Client.java 
---
@@ -451,7 +451,6 @@ public int getPort() {
 public void close() {
 if (!closing) {
 LOG.info("closing Netty Client {}", dstAddressPrefixedName);
-context.removeClient(dstHost, dstAddress.getPort());
--- End diff --

done


---

[GitHub] storm pull request #2669: [STORM-3055] remove conext connection cache

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2669#discussion_r188055495
  
--- Diff: storm-client/src/jvm/org/apache/storm/messaging/netty/Client.java 
---
@@ -451,7 +451,6 @@ public int getPort() {
 public void close() {
 if (!closing) {
 LOG.info("closing Netty Client {}", dstAddressPrefixedName);
-context.removeClient(dstHost, dstAddress.getPort());
--- End diff --

This line is the only reason to pass context into the Client.  Please 
remove all references to context if we are going to remove this.


---

[GitHub] storm pull request #2669: [STORM-3055] remove conext connection cache

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2669#discussion_r188056330
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/messaging/netty/Context.java ---
@@ -65,20 +65,8 @@ public synchronized IConnection bind(String storm_id, 
int port) {
  * establish a connection to a remote server
  */
 public synchronized IConnection connect(String storm_id, String host, 
int port, AtomicBoolean[] remoteBpStatus) {
-IConnection connection = connections.get(key(host, port));
--- End diff --

Please rename connections to `serverConnections` as it is only for Servers 
now.  Also it would be nice to change it back to a list or a set instead of a 
map, because we will no longer be using it as a cache, so we don't need to pull 
out a specific value any longer.  We just want it so we can close them on 
termination.


---

[GitHub] storm pull request #2669: [STORM-3055] remove conext connection cache

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2669#discussion_r188055726
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/messaging/netty/Context.java ---
@@ -65,20 +65,8 @@ public synchronized IConnection bind(String storm_id, 
int port) {
  * establish a connection to a remote server
  */
 public synchronized IConnection connect(String storm_id, String host, 
int port, AtomicBoolean[] remoteBpStatus) {
--- End diff --

This no longer needs to be synchronized.  That was used to protect 
connections, and is no longer needed if all we do is create a new Client.


---

[GitHub] storm pull request #2669: [STORM-3055] remove conext connection cache

[pczb]

GitHub user pczb opened a pull request:

https://github.com/apache/storm/pull/2669

[STORM-3055] remove conext connection cache

workerState has already cache connection use supervisor_id + port
and if context and worker both cache connection, there can be some problem 
describe in jira
jira link: https://issues.apache.org/jira/browse/STORM-3055

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/pczb/storm dev

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/2669.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 #2669


commit 62f66cd3f1e39953391f36e2b01197a6bade4e5c
Author: pczb 
Date:   2018-05-09T17:41:18Z

remove context connection

Signed-off-by: pczb 




---