You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/01/08 18:41:07 UTC
[GitHub] [accumulo] Manno15 opened a new pull request #1470: Fix CleanUpIT.
Fixes #1449
Manno15 opened a new pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470
CleanUpIt was failing due to 'Thread pool checker' not being closed properly. With this change, the thread is picked up properly and closed as expected.
#1449
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] Manno15 commented on a change in pull request #1470: Fix
CleanUpIT. Fixes #1449
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#discussion_r367166531
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -861,10 +849,18 @@ public void startCheckerThread() {
}
void closeExpiredConnections() {
- List<CachedConnection> expiredConnections;
-
+ ArrayList<CachedConnection> expiredConnections = new ArrayList<>();
ConnectionPool pool = getConnectionPool();
- expiredConnections = pool.removeExpiredConnections(killTime);
+
+ for (Entry<ThriftTransportKey,CachedConnections> entry : pool.connections.entrySet()) {
+ Iterator<CachedConnection> iter = entry.getValue().unreserved.iterator();
+ while (iter.hasNext()) {
+ pool.executeWithinLock(entry.getKey(), (key) -> {
+ entry.getValue().removeExpiredConnections(expiredConnections, killTime);
+ entry.getValue().checkReservedForStuckIO();
+ });
+ }
+ }
Review comment:
I will take a look at your changes. My goal was to fix the bug without reverting too much of the refactor that was done to it. In an older release, a similar method was done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] Manno15 commented on a change in pull request #1470: Fix
CleanUpIT. Fixes #1449
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#discussion_r364701816
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -861,10 +849,18 @@ public void startCheckerThread() {
}
void closeExpiredConnections() {
- List<CachedConnection> expiredConnections;
-
+ ArrayList<CachedConnection> expiredConnections = new ArrayList<>();
ConnectionPool pool = getConnectionPool();
- expiredConnections = pool.removeExpiredConnections(killTime);
+
+ for (Entry<ThriftTransportKey,CachedConnections> entry : pool.connections.entrySet()) {
+ Iterator<CachedConnection> iter = entry.getValue().unreserved.iterator();
+ while (iter.hasNext()) {
+ pool.executeWithinLock(entry.getKey(), (key) -> {
+ entry.getValue().removeExpiredConnections(expiredConnections, killTime);
+ entry.getValue().checkReservedForStuckIO();
+ });
+ }
+ }
Review comment:
This was similar to how it was handled before the refactoring. Without the Iter, the thread never gets picked up and removed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] Manno15 commented on a change in pull request #1470: Fix
CleanUpIT. Fixes #1449
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#discussion_r367166731
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -861,10 +849,18 @@ public void startCheckerThread() {
}
void closeExpiredConnections() {
- List<CachedConnection> expiredConnections;
-
+ ArrayList<CachedConnection> expiredConnections = new ArrayList<>();
ConnectionPool pool = getConnectionPool();
- expiredConnections = pool.removeExpiredConnections(killTime);
+
+ for (Entry<ThriftTransportKey,CachedConnections> entry : pool.connections.entrySet()) {
+ Iterator<CachedConnection> iter = entry.getValue().unreserved.iterator();
+ while (iter.hasNext()) {
+ pool.executeWithinLock(entry.getKey(), (key) -> {
+ entry.getValue().removeExpiredConnections(expiredConnections, killTime);
+ entry.getValue().checkReservedForStuckIO();
+ });
+ }
+ }
Review comment:
I will take a look at your changes. My goal was to fix the bug without reverting too much of the refactor that was done to it. In an older release, a similar method was done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1470: Fix CleanUpIT. Fixes #1449
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#discussion_r367053321
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -861,10 +849,18 @@ public void startCheckerThread() {
}
void closeExpiredConnections() {
- List<CachedConnection> expiredConnections;
-
+ ArrayList<CachedConnection> expiredConnections = new ArrayList<>();
ConnectionPool pool = getConnectionPool();
- expiredConnections = pool.removeExpiredConnections(killTime);
+
+ for (Entry<ThriftTransportKey,CachedConnections> entry : pool.connections.entrySet()) {
+ Iterator<CachedConnection> iter = entry.getValue().unreserved.iterator();
+ while (iter.hasNext()) {
+ pool.executeWithinLock(entry.getKey(), (key) -> {
+ entry.getValue().removeExpiredConnections(expiredConnections, killTime);
+ entry.getValue().checkReservedForStuckIO();
+ });
+ }
+ }
Review comment:
I was having a hard time understanding this change and I wanted to discuss it at the hackathon yesterday, but was not able to make it. I attempted to fix this issue in #1477 because this seemed like the quickest way to share ideas.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] Manno15 commented on a change in pull request #1470: Fix
CleanUpIT. Fixes #1449
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#discussion_r364754828
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -861,10 +849,18 @@ public void startCheckerThread() {
}
void closeExpiredConnections() {
- List<CachedConnection> expiredConnections;
-
+ ArrayList<CachedConnection> expiredConnections = new ArrayList<>();
ConnectionPool pool = getConnectionPool();
- expiredConnections = pool.removeExpiredConnections(killTime);
+
+ for (Entry<ThriftTransportKey,CachedConnections> entry : pool.connections.entrySet()) {
+ Iterator<CachedConnection> iter = entry.getValue().unreserved.iterator();
+ while (iter.hasNext()) {
+ pool.executeWithinLock(entry.getKey(), (key) -> {
+ entry.getValue().removeExpiredConnections(expiredConnections, killTime);
+ entry.getValue().checkReservedForStuckIO();
+ });
+ }
+ }
Review comment:
There are a couple of other loop options that seem to work but none that are just the for loop.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii closed pull request #1470: Fix CleanUpIT. Fixes
#1449
Posted by GitBox <gi...@apache.org>.
ctubbsii closed pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1470: Fix CleanUpIT. Fixes
#1449
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#issuecomment-575412159
Closing this. It was superseded by #1477.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1470: Fix CleanUpIT. Fixes #1449
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#discussion_r367053321
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -861,10 +849,18 @@ public void startCheckerThread() {
}
void closeExpiredConnections() {
- List<CachedConnection> expiredConnections;
-
+ ArrayList<CachedConnection> expiredConnections = new ArrayList<>();
ConnectionPool pool = getConnectionPool();
- expiredConnections = pool.removeExpiredConnections(killTime);
+
+ for (Entry<ThriftTransportKey,CachedConnections> entry : pool.connections.entrySet()) {
+ Iterator<CachedConnection> iter = entry.getValue().unreserved.iterator();
+ while (iter.hasNext()) {
+ pool.executeWithinLock(entry.getKey(), (key) -> {
+ entry.getValue().removeExpiredConnections(expiredConnections, killTime);
+ entry.getValue().checkReservedForStuckIO();
+ });
+ }
+ }
Review comment:
I was having a hard time understanding this change. I attempted to fix this issue in #1477
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] Manno15 commented on a change in pull request #1470: Fix
CleanUpIT. Fixes #1449
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#discussion_r367166531
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -861,10 +849,18 @@ public void startCheckerThread() {
}
void closeExpiredConnections() {
- List<CachedConnection> expiredConnections;
-
+ ArrayList<CachedConnection> expiredConnections = new ArrayList<>();
ConnectionPool pool = getConnectionPool();
- expiredConnections = pool.removeExpiredConnections(killTime);
+
+ for (Entry<ThriftTransportKey,CachedConnections> entry : pool.connections.entrySet()) {
+ Iterator<CachedConnection> iter = entry.getValue().unreserved.iterator();
+ while (iter.hasNext()) {
+ pool.executeWithinLock(entry.getKey(), (key) -> {
+ entry.getValue().removeExpiredConnections(expiredConnections, killTime);
+ entry.getValue().checkReservedForStuckIO();
+ });
+ }
+ }
Review comment:
I will take a look at your changes. My goal was to fix the bug without reverting too much of the refactor that was done to it. In an older release, a similar method was done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] Manno15 commented on a change in pull request #1470: Fix
CleanUpIT. Fixes #1449
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#discussion_r364754828
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -861,10 +849,18 @@ public void startCheckerThread() {
}
void closeExpiredConnections() {
- List<CachedConnection> expiredConnections;
-
+ ArrayList<CachedConnection> expiredConnections = new ArrayList<>();
ConnectionPool pool = getConnectionPool();
- expiredConnections = pool.removeExpiredConnections(killTime);
+
+ for (Entry<ThriftTransportKey,CachedConnections> entry : pool.connections.entrySet()) {
+ Iterator<CachedConnection> iter = entry.getValue().unreserved.iterator();
+ while (iter.hasNext()) {
+ pool.executeWithinLock(entry.getKey(), (key) -> {
+ entry.getValue().removeExpiredConnections(expiredConnections, killTime);
+ entry.getValue().checkReservedForStuckIO();
+ });
+ }
+ }
Review comment:
There are a couple of other loop options that seem to work but none that are just the for loop.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1470:
Fix CleanUpIT. Fixes #1449
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1470: Fix CleanUpIT. Fixes #1449
URL: https://github.com/apache/accumulo/pull/1470#discussion_r364514319
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -861,10 +849,18 @@ public void startCheckerThread() {
}
void closeExpiredConnections() {
- List<CachedConnection> expiredConnections;
-
+ ArrayList<CachedConnection> expiredConnections = new ArrayList<>();
ConnectionPool pool = getConnectionPool();
- expiredConnections = pool.removeExpiredConnections(killTime);
+
+ for (Entry<ThriftTransportKey,CachedConnections> entry : pool.connections.entrySet()) {
+ Iterator<CachedConnection> iter = entry.getValue().unreserved.iterator();
+ while (iter.hasNext()) {
+ pool.executeWithinLock(entry.getKey(), (key) -> {
+ entry.getValue().removeExpiredConnections(expiredConnections, killTime);
+ entry.getValue().checkReservedForStuckIO();
+ });
+ }
+ }
Review comment:
I don't understand why this is iterating over the `pool.connections.entrySet()`, but is checking for a non-empty iterator from `entry.getValue().unreserved`. Is the `iter.hasNext()` even necessary? The iterator doesn't seem to be advanced by calling `next()` ever. Is that check performing something important?
----------------------------------------------------------------
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
With regards,
Apache Git Services