aajisaka commented on a change in pull request #3080:
URL: https://github.com/apache/hadoop/pull/3080#discussion_r663396087
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java
##########
@@ -426,34 +408,41 @@ private synchronized SelectorInfo get(SelectableChannel
channel)
*
* @param info
*/
- private synchronized void release(SelectorInfo info) {
+ private static void release(SelectorInfo info) {
long now = Time.now();
trimIdleSelectors(now);
info.lastActivityTime = now;
- info.queue.addLast(info);
+ // SelectorInfos in queue are sorted by lastActivityTime
+ providerMap.get(info.provider).addLast(info);
}
+ private static AtomicBoolean trimming = new AtomicBoolean(false);
+ private static volatile long lastTrimTime = Time.now();
Review comment:
`lastTrimTime` is unused.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java
##########
@@ -426,34 +408,41 @@ private synchronized SelectorInfo get(SelectableChannel
channel)
*
* @param info
*/
- private synchronized void release(SelectorInfo info) {
+ private static void release(SelectorInfo info) {
long now = Time.now();
trimIdleSelectors(now);
info.lastActivityTime = now;
- info.queue.addLast(info);
+ // SelectorInfos in queue are sorted by lastActivityTime
+ providerMap.get(info.provider).addLast(info);
}
+ private static AtomicBoolean trimming = new AtomicBoolean(false);
+ private static volatile long lastTrimTime = Time.now();
+
/**
* Closes selectors that are idle for IDLE_TIMEOUT (10 sec). It does not
* traverse the whole list, just over the one that have crossed
* the timeout.
*/
- private void trimIdleSelectors(long now) {
+ private static void trimIdleSelectors(long now) {
+ if (!trimming.compareAndSet(false, true)) {
+ return;
+ }
+
long cutoff = now - IDLE_TIMEOUT;
-
- for(ProviderInfo pList=providerList; pList != null; pList=pList.next) {
- if (pList.queue.isEmpty()) {
- continue;
- }
- for(Iterator<SelectorInfo> it = pList.queue.iterator(); it.hasNext();)
{
- SelectorInfo info = it.next();
- if (info.lastActivityTime > cutoff) {
+ for (ConcurrentLinkedDeque<SelectorInfo> infoQ : providerMap.values()) {
+ SelectorInfo oldest;
+ while ((oldest = infoQ.peekFirst()) != null) {
+ if (oldest.lastActivityTime <= cutoff && infoQ.remove(oldest)) {
Review comment:
Given the first element of `infoQ` is not changed between
`infoQ.peelFirst()` and `infoQ.remove(oldest)`, I think `infoQ.remove(oldest)`
can be replaced with `infoQ.removeFirst()`.
```suggestion
if (oldest.lastActivityTime <= cutoff) {
// Given there are no other threads that change the first
element,
// we can safely remove the oldest from infoQ by removeFirst();
infoQ.removeFirst();
```
--
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.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
