dlmarion commented on a change in pull request #2422:
URL: https://github.com/apache/accumulo/pull/2422#discussion_r791015464
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchReaderIterator.java
##########
@@ -497,26 +499,44 @@ private void
doLookups(Map<String,Map<KeyExtent,List<Range>>> binnedRanges,
for (final String tsLocation : locations) {
final Map<KeyExtent,List<Range>> tabletsRanges =
binnedRanges.get(tsLocation);
- if (maxTabletsPerRequest == Integer.MAX_VALUE || tabletsRanges.size() ==
1) {
- QueryTask queryTask = new QueryTask(tsLocation, tabletsRanges,
failures, receiver, columns);
- queryTasks.add(queryTask);
+ if (options.isUseScanServer()) {
+ // Ignore the tablets location and find a scan server to use
+ ScanServerLocator ssl = context.getScanServerLocator();
+ tabletsRanges.forEach((k, v) -> {
+ try {
+ String location = ssl.reserveScanServer(new TabletIdImpl(k));
Review comment:
As currently implemented a ScanServer performs one scan at a time, much
like the Compactor performs one compaction at a time. I figured that if a
ScanServer had many threads and performed more than one scan at a time, then we
would potentially run into the same situation we have in in the TabletServer
w/r/t memory usage.
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchReaderIterator.java
##########
@@ -497,26 +499,44 @@ private void
doLookups(Map<String,Map<KeyExtent,List<Range>>> binnedRanges,
for (final String tsLocation : locations) {
final Map<KeyExtent,List<Range>> tabletsRanges =
binnedRanges.get(tsLocation);
- if (maxTabletsPerRequest == Integer.MAX_VALUE || tabletsRanges.size() ==
1) {
- QueryTask queryTask = new QueryTask(tsLocation, tabletsRanges,
failures, receiver, columns);
- queryTasks.add(queryTask);
+ if (options.isUseScanServer()) {
+ // Ignore the tablets location and find a scan server to use
+ ScanServerLocator ssl = context.getScanServerLocator();
+ tabletsRanges.forEach((k, v) -> {
+ try {
+ String location = ssl.reserveScanServer(new TabletIdImpl(k));
Review comment:
Another thought w/r/t to the load balancer code and the number RPCs to
ZK, we could move the logic into the Manager and reduce the ZK traffic with
ZooCache. I'm not tied to the current approach, I was just trying to make a
simple reservation system that could be replaced with a user supplied class
that performs more complex logic.
> We could start off implementing the thread pool with syncQ with a hard
coded thread of size of one and get this really fast busy exception behavior.
I'm good with that. The Scan Server already returns an error if its working
on a scan for another client. I'm currently working on some more ITs and
running down an issue I am seeing in one of them. Let's talk more about
potential design changes.
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchReaderIterator.java
##########
@@ -497,26 +499,44 @@ private void
doLookups(Map<String,Map<KeyExtent,List<Range>>> binnedRanges,
for (final String tsLocation : locations) {
final Map<KeyExtent,List<Range>> tabletsRanges =
binnedRanges.get(tsLocation);
- if (maxTabletsPerRequest == Integer.MAX_VALUE || tabletsRanges.size() ==
1) {
- QueryTask queryTask = new QueryTask(tsLocation, tabletsRanges,
failures, receiver, columns);
- queryTasks.add(queryTask);
+ if (options.isUseScanServer()) {
+ // Ignore the tablets location and find a scan server to use
+ ScanServerLocator ssl = context.getScanServerLocator();
+ tabletsRanges.forEach((k, v) -> {
+ try {
+ String location = ssl.reserveScanServer(new TabletIdImpl(k));
Review comment:
My latest commit added more tests in ScanServerIT. I need to add a
couple more tests that involve the BatchScanner, but I'm pretty well convinced
that this approach will work. I think the ScanServer locator piece might be the
most complex piece about this change when we are done with it. From a load
balancer perspective, it would be nice to know the following for each
ScanServer:
- last N extents scanned (increased probability of cache hit?).
- local TabletServers Tablets, if any (increased probability of locality or
page cache hit?)
The extent information is likely sensitive and not something we want
client-side. If we choose to use this information for load balancing, then it
might have to go into the Manager.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
