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 2021/09/20 20:32:30 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2279: Improve fetchScans in Monitor
milleruntime opened a new pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279
* Drop code that was trying to remove scans older than 5 mins
* Active scans tracking is at the mercy of the server so let it determine
what gets put in the monitor map
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2279: Improve fetchScans in Monitor
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279#issuecomment-923276963
Was this code added because the TServer was reporting scans that were not actively running?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime closed pull request #2279: Improve fetchScans in Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime closed pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2279: Improve fetchScans in Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279#issuecomment-924240749
Nevermind, this is not needed since its in the same method that adds to the map.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2279: Improve fetchScans in Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279#issuecomment-924232325
I changed this to make the remove code more thread friendly
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2279: Improve fetchScans in Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279#issuecomment-923297372
> Was this code added because the TServer was reporting scans that were not actively running?
It was a part of the initial creation of the method. The method was added to do the opposite (report long running scans) to help debug bigger problems with a cluster in commit 7f329a9c4dbf6f249a14a1f4bde43f261bf12875. See https://issues.apache.org/jira/browse/ACCUMULO-2645. But the code to age off old scans isn't thread safe and I am not sure why you would just remove a scan only 5 minutes later after reporting it. If an admin doesn't see the scan in that window, its useless to put it on the monitor. If there are long running scans being returned than I think they should get displayed. It is disingenuous to report no scans running if there are.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime closed pull request #2279: Improve fetchScans in Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime closed pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2279: Improve fetchScans in Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279#discussion_r713010754
##########
File path: server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java
##########
@@ -578,15 +577,6 @@ private void fetchScans() throws Exception {
ThriftUtil.returnClient(tserver);
}
}
- // Age off old scan information
- Iterator<Entry<HostAndPort,ScanStats>> entryIter = allScans.entrySet().iterator();
- long now = System.currentTimeMillis();
- while (entryIter.hasNext()) {
- Entry<HostAndPort,ScanStats> entry = entryIter.next();
- if (now - entry.getValue().fetched > 5 * 60 * 1000) {
- entryIter.remove();
- }
- }
Review comment:
The map will only grow per tserver. For each tserver, we get the active scans and do a straight put in the map:
<pre>
allScans.put(parsedServer, new ScanStats(scans));
</pre>
So yes it could grow as tservers are added and removed from the cluster but not by that much. Either way, this could be done better so it only stores the relevant data.
I am trying to simulate a slow running scan in Uno using `SlowIterator`. If I can get that to work, I will make this better.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2279: Improve fetchScans in Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279#issuecomment-924021561
I was able to simulate a long running scan using SlowIterator with these two shell commands:
<pre>
config -t test_ingest -s table.iterator.scan.slow=1,org.apache.accumulo.test.functional.SlowIterator
config -t test_ingest -s table.iterator.scan.slow.opt.sleepTime=1000
</pre>
I was wrong about this chunk of code. It isn't thread safe but it does only remove inactive tservers from the `allScans` map. It sets the `fetched` value every time it gets an active scan for that tserver. So when there are no more active scans for that tserver, it gets dropped from `allScans`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2279: Improve fetchScans in Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2279:
URL: https://github.com/apache/accumulo/pull/2279#discussion_r712661601
##########
File path: server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java
##########
@@ -578,15 +577,6 @@ private void fetchScans() throws Exception {
ThriftUtil.returnClient(tserver);
}
}
- // Age off old scan information
- Iterator<Entry<HostAndPort,ScanStats>> entryIter = allScans.entrySet().iterator();
- long now = System.currentTimeMillis();
- while (entryIter.hasNext()) {
- Entry<HostAndPort,ScanStats> entry = entryIter.next();
- if (now - entry.getValue().fetched > 5 * 60 * 1000) {
- entryIter.remove();
- }
- }
Review comment:
I don't think this change will result in desired behavior. There is no other code that will ever remove anything from `allScans` other than this code. So, that object will continue to grow indefinitely, keeping every scan ever fetched, until the monitor falls over due to memory problems.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org