You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/04/12 20:54:11 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3292: fixes #3235 support tablets w/ and w/o location in tablet location cache
keith-turner opened a new pull request, #3292:
URL: https://github.com/apache/accumulo/pull/3292
Modifies the TabletLocatorCache to support tablets with and without a location. Also modified the ScanServerSelector to support optionally waiting for scan servers when there are none instead of always falling back to tservers. The scan severs selector falling back to the tserver was written with the assumption that tablets are always hosted. Now that tablets are loaded onto tserers on demand it may not be desirable to load the tablets when there are no scan servers. That is why this changes also included changes to optionally wait for scan servers when there are none. Restarting all scan servers will be a normal administrative operations, may not want a lot of tablets to all of a sudden load on tablet servers when the scan servers are restarted.
--
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] keith-turner merged pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3292:
URL: https://github.com/apache/accumulo/pull/3292
--
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 a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1169966499
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/RootTabletLocator.java:
##########
@@ -132,10 +138,14 @@ protected TabletLocation getRootTabletLocation(ClientContext context) {
@Override
public TabletLocation locateTablet(ClientContext context, Text row, boolean skipRow,
- boolean retry) {
+ HostingNeed hostingNeed) {
+ // only expect the hosted case so this code only handles that, so throw an exception is
+ // something else is seed
Review Comment:
Comment is confusing. Why check the hostingNeed variable if it's never used in the method? Is it to prevent bugs in other parts of the codebase?
--
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 #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#issuecomment-1507144819
https://github.com/apache/accumulo/pull/3294
--
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 a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1171814885
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -299,10 +305,52 @@ public ScanState(ClientContext context, TableId tableId, Authorizations authoriz
}
}
- public static class ScanTimedOutException extends IOException {
+ static <T> Optional<T> waitUntil(Supplier<Optional<T>> condition, Duration maxWaitTime,
Review Comment:
If it were me, I would do the following in `main`:
1. Move `Wait` to `org.apache.accumulo.core.util`
2. Move `UtilWaitThread.sleep` into `Wait`
3. Add this method to `Wait`
--
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] keith-turner commented on a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1170646911
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java:
##########
@@ -58,33 +61,81 @@ boolean isValid() {
return isValid;
}
+ /**
+ * Used to indicate if a user of this interface needs a tablet hosted or not. This simple enum was
+ * created instead of using a boolean for code clarity.
+ */
+ public enum HostingNeed {
+ HOSTED, NONE
+ }
+
+ // ELASTICITY_TODO rename to findTablet
+ /**
+ * Finds the tablet that contains the given row.
+ *
+ * @param hostingNeed When {@link HostingNeed#HOSTED} is passed will only return a tablet if it
+ * has location. When {@link HostingNeed#NONE} is passed will return the tablet that
+ * overlaps the row.
+ *
+ * @return overlapping tablet. If no overlapping tablet exists, returns null. If hosting is
+ * required and the tablet currently has no location ,returns null.
+ */
public abstract TabletLocation locateTablet(ClientContext context, Text row, boolean skipRow,
- boolean retry) throws AccumuloException, AccumuloSecurityException, TableNotFoundException;
+ HostingNeed hostingNeed)
+ throws AccumuloException, AccumuloSecurityException, TableNotFoundException;
+
+ public TabletLocation locateTabletWithRetry(ClientContext context, Text row, boolean skipRow,
Review Comment:
There were many (maybe 11) cases passing false for retry and a few (maybe 2) passing true. So when I was making changes I decided pulling the retry out simplified the code. The code that was passing false for retry was doing its own thing and in some cases did not want to retry forever (like scan timeout setting) or wanted to do other stuff in the retry 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.
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 a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1169965422
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/RootTabletLocator.java:
##########
@@ -57,19 +59,23 @@ public <T extends Mutation> void binMutations(ClientContext context, List<T> mut
Map<String,TabletServerMutations<T>> binnedMutations, List<T> failures) {
TabletLocation rootTabletLocation = getRootTabletLocation(context);
if (rootTabletLocation != null) {
- var tsm = new TabletServerMutations<T>(rootTabletLocation.getTserverSession());
+ var tsm = new TabletServerMutations<T>(rootTabletLocation.getTserverSession().get());
for (T mutation : mutations) {
tsm.addMutation(RootTable.EXTENT, mutation);
}
- binnedMutations.put(rootTabletLocation.getTserverLocation(), tsm);
+ binnedMutations.put(rootTabletLocation.getTserverLocation().get(), tsm);
} else {
failures.addAll(mutations);
}
}
@Override
public List<Range> locateTablets(ClientContext context, List<Range> ranges,
- BiConsumer<TabletLocation,Range> rangeConsumer) {
+ BiConsumer<TabletLocation,Range> rangeConsumer, HostingNeed hostingNeed) {
+
+ // only expect the hosted case so this code only handles that, so throw an exception is
+ // something else is seed
Review Comment:
Comment is confusing. Why check the hostingNeed variable if it's never used in the method? Is it to prevent bugs in other parts of the codebase?
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocatorImpl.java:
##########
@@ -584,22 +600,45 @@ private void requestTabletHosting(ClientContext context, Range range)
return;
}
- String tableName = context.getTableName(tableId);
- if (!context.tableOperations().isOnline(tableName)) {
- log.trace("requestTabletHosting: table {} is not online", tableId);
+ if (extentsWithNoLocation.isEmpty()) {
return;
}
- List<TKeyExtent> extentsToBringOnline =
- findExtentsForRange(context, tableId, range, Set.of(TabletHostingGoal.NEVER), true);
- if (extentsToBringOnline.isEmpty()) {
+ if (context.getTableState(tableId) != TableState.ONLINE) {
+ log.trace("requestTabletHosting: table {} is not online", tableId);
return;
}
- log.debug("Requesting tablets be hosted: {}", extentsToBringOnline);
- ThriftClientTypes.TABLET_MGMT.executeVoid(context,
- client -> client.requestTabletHosting(TraceUtil.traceInfo(), context.rpcCreds(),
- tableId.canonical(), extentsToBringOnline));
- tabletHostingRequestCount.addAndGet(extentsToBringOnline.size());
+
+ List<KeyExtent> extentsToLookup = new ArrayList<>();
+ for (var extent : extentsWithNoLocation) {
+ if (recentOndemandRequest.asMap().putIfAbsent(extent, System.currentTimeMillis()) == null) {
+ extentsToLookup.add(extent);
+ log.debug("Marking tablet as onDemand: {}", extent);
+ }
+ }
+
+ List<TKeyExtent> extentsToBringOnline = new ArrayList<>();
+
+ for (TabletMetadata tabletMetadata : context.getAmple().readTablets()
+ .forTablets(extentsToLookup).fetch(HOSTING_REQUESTED, HOSTING_GOAL).build()) {
+ if (tabletMetadata.getHostingGoal() == TabletHostingGoal.ONDEMAND
+ && !tabletMetadata.getHostingRequested()) {
+ extentsToBringOnline.add(tabletMetadata.getExtent().toThrift());
+ }
+
+ if (tabletMetadata.getHostingGoal() == TabletHostingGoal.NEVER) {
+ throw new AccumuloException("Extent " + tabletMetadata.getExtent()
+ + " has a tablet hosting goal state " + TabletHostingGoal.NEVER);
+ }
+ }
+
+ if (!extentsToBringOnline.isEmpty()) {
+ log.debug("Requesting tablets be hosted: {}", extentsToBringOnline);
+ ThriftClientTypes.TABLET_MGMT.executeVoid(context,
+ client -> client.requestTabletHosting(TraceUtil.traceInfo(), context.rpcCreds(),
+ tableId.canonical(), extentsToBringOnline));
+ tabletHostingRequestCount.addAndGet(extentsToBringOnline.size());
+ }
}
public static List<TKeyExtent> findExtentsForRange(ClientContext context, TableId tableId,
Review Comment:
IIRC `findExtentsForRange` is called from two places. You just removed one with the changes above. So, it's possible that we could move this method.
--
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] keith-turner commented on a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1170635870
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/RootTabletLocator.java:
##########
@@ -57,19 +59,23 @@ public <T extends Mutation> void binMutations(ClientContext context, List<T> mut
Map<String,TabletServerMutations<T>> binnedMutations, List<T> failures) {
TabletLocation rootTabletLocation = getRootTabletLocation(context);
if (rootTabletLocation != null) {
- var tsm = new TabletServerMutations<T>(rootTabletLocation.getTserverSession());
+ var tsm = new TabletServerMutations<T>(rootTabletLocation.getTserverSession().get());
for (T mutation : mutations) {
tsm.addMutation(RootTable.EXTENT, mutation);
}
- binnedMutations.put(rootTabletLocation.getTserverLocation(), tsm);
+ binnedMutations.put(rootTabletLocation.getTserverLocation().get(), tsm);
} else {
failures.addAll(mutations);
}
}
@Override
public List<Range> locateTablets(ClientContext context, List<Range> ranges,
- BiConsumer<TabletLocation,Range> rangeConsumer) {
+ BiConsumer<TabletLocation,Range> rangeConsumer, HostingNeed hostingNeed) {
+
+ // only expect the hosted case so this code only handles that, so throw an exception is
+ // something else is seed
Review Comment:
Yeah, was only expecting request for the root tablet to have a location. Also there is no handling for the other cases, like the root tablet will not be requested to be brought online.
--
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] keith-turner commented on a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1172004877
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -299,10 +305,52 @@ public ScanState(ClientContext context, TableId tableId, Authorizations authoriz
}
}
- public static class ScanTimedOutException extends IOException {
+ static <T> Optional<T> waitUntil(Supplier<Optional<T>> condition, Duration maxWaitTime,
Review Comment:
This method has some things very specific to scans in it so it could not be moved as is. However it could build on a more generic method and pass the scan specific code in as a lambda.
--
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] keith-turner commented on a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1170662735
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocatorImpl.java:
##########
@@ -584,22 +600,45 @@ private void requestTabletHosting(ClientContext context, Range range)
return;
}
- String tableName = context.getTableName(tableId);
- if (!context.tableOperations().isOnline(tableName)) {
- log.trace("requestTabletHosting: table {} is not online", tableId);
+ if (extentsWithNoLocation.isEmpty()) {
return;
}
- List<TKeyExtent> extentsToBringOnline =
- findExtentsForRange(context, tableId, range, Set.of(TabletHostingGoal.NEVER), true);
- if (extentsToBringOnline.isEmpty()) {
+ if (context.getTableState(tableId) != TableState.ONLINE) {
+ log.trace("requestTabletHosting: table {} is not online", tableId);
return;
}
- log.debug("Requesting tablets be hosted: {}", extentsToBringOnline);
- ThriftClientTypes.TABLET_MGMT.executeVoid(context,
- client -> client.requestTabletHosting(TraceUtil.traceInfo(), context.rpcCreds(),
- tableId.canonical(), extentsToBringOnline));
- tabletHostingRequestCount.addAndGet(extentsToBringOnline.size());
+
+ List<KeyExtent> extentsToLookup = new ArrayList<>();
+ for (var extent : extentsWithNoLocation) {
+ if (recentOndemandRequest.asMap().putIfAbsent(extent, System.currentTimeMillis()) == null) {
+ extentsToLookup.add(extent);
+ log.debug("Marking tablet as onDemand: {}", extent);
+ }
+ }
+
+ List<TKeyExtent> extentsToBringOnline = new ArrayList<>();
+
+ for (TabletMetadata tabletMetadata : context.getAmple().readTablets()
+ .forTablets(extentsToLookup).fetch(HOSTING_REQUESTED, HOSTING_GOAL).build()) {
+ if (tabletMetadata.getHostingGoal() == TabletHostingGoal.ONDEMAND
+ && !tabletMetadata.getHostingRequested()) {
+ extentsToBringOnline.add(tabletMetadata.getExtent().toThrift());
+ }
+
+ if (tabletMetadata.getHostingGoal() == TabletHostingGoal.NEVER) {
+ throw new AccumuloException("Extent " + tabletMetadata.getExtent()
+ + " has a tablet hosting goal state " + TabletHostingGoal.NEVER);
+ }
+ }
+
+ if (!extentsToBringOnline.isEmpty()) {
+ log.debug("Requesting tablets be hosted: {}", extentsToBringOnline);
+ ThriftClientTypes.TABLET_MGMT.executeVoid(context,
+ client -> client.requestTabletHosting(TraceUtil.traceInfo(), context.rpcCreds(),
+ tableId.canonical(), extentsToBringOnline));
+ tabletHostingRequestCount.addAndGet(extentsToBringOnline.size());
+ }
}
public static List<TKeyExtent> findExtentsForRange(ClientContext context, TableId tableId,
Review Comment:
I moved it to TableOperationsImpl.
After consulting the cache we know what extents have no location, so those extents are requested to be hosted if needed.
--
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] keith-turner commented on pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#issuecomment-1507126735
> Some of the things in the PR may not be necessary given the changes that I am working on to make all user tables on-demand by default. I'm close to being ready to create a PR for that, I have some things I need to finish first (looking at tests, testing locally, etc.). If you want an early look at my changes, see https://github.com/apache/accumulo/compare/elasticity...dlmarion:accumulo:elasticity-ondemand-feature-changes2?expand=1.
There will be conflicts. I took a look and had some thoughts, it you post a draft PR I could make a few comments.
--
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 a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1171793144
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchReaderIterator.java:
##########
@@ -679,32 +686,55 @@ public Collection<? extends ScanServerAttempt> getAttempts(TabletId tabletId) {
public Map<String,String> getHints() {
return options.executionHints;
}
+
+ @Override
+ public <T> Optional<T> waitUntil(Supplier<Optional<T>> condition, Duration maxWaitTime,
+ String description) {
+ return ThriftScanner.waitUntil(condition, maxWaitTime, description, timeoutLeft, context,
+ tableId, log);
+ }
};
var actions = ecsm.selectServers(params);
Map<String,ScanServerAttemptReporter> reporters = new HashMap<>();
+ failures = new ArrayList<>();
+
for (TabletIdImpl tabletId : tabletIds) {
KeyExtent extent = tabletId.toKeyExtent();
String serverToUse = actions.getScanServer(tabletId);
if (serverToUse == null) {
// no scan server was given so use the tablet server
- serverToUse = Objects.requireNonNull(extentToTserverMap.get(extent));
- log.trace("For tablet {} scan server selector chose tablet_server", tabletId);
+ serverToUse = extentToTserverMap.get(extent);
+ if (serverToUse != null) {
+ log.trace("For tablet {} scan server selector chose tablet_server", tabletId);
Review Comment:
```suggestion
log.trace("For tablet {} scan server selector chose tablet_server {}", tabletId, serverToUse);
```
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -299,10 +305,52 @@ public ScanState(ClientContext context, TableId tableId, Authorizations authoriz
}
}
- public static class ScanTimedOutException extends IOException {
+ static <T> Optional<T> waitUntil(Supplier<Optional<T>> condition, Duration maxWaitTime,
Review Comment:
I wonder if the `Wait` class should be promoted out of the test package and this added to that class, or this added to `UtilWaitThread`.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -369,10 +427,19 @@ public Map<String,String> getHints() {
log.trace("For tablet {} scan server selector chose scan_server:{} delay:{} busyTimeout:{}",
loc.getExtent(), scanServer, delay, scanState.busyTimeout);
} else {
- addr = new ScanAddress(loc.getTserverLocation(), ServerType.TSERVER, loc);
- delay = actions.getDelay();
- scanState.busyTimeout = Duration.ZERO;
- log.trace("For tablet {} scan server selector chose tablet_server", loc.getExtent());
+ Optional<String> tserverLoc = loc.getTserverLocation();
+
+ if (tserverLoc.isPresent()) {
+ addr = new ScanAddress(loc.getTserverLocation().get(), ServerType.TSERVER, loc);
+ delay = actions.getDelay();
+ scanState.busyTimeout = Duration.ZERO;
+ log.trace("For tablet {} scan server selector chose tablet_server", loc.getExtent());
Review Comment:
```suggestion
log.trace("For tablet {} scan server selector chose tablet_server: {}", loc.getExtent(), addr);
```
--
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 #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#issuecomment-1511378526
Full IT build completed successfully
--
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 #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#issuecomment-1506858142
I kicked off a fuil IT build. Some of the things in the PR may not be necessary given the changes that I am working on to make all user tables on-demand by default. I'm close to being ready to create a PR for that, I have some things I need to finish first (looking at tests, testing locally, etc.). If you want an early look at my changes, see https://github.com/apache/accumulo/compare/elasticity...dlmarion:accumulo:elasticity-ondemand-feature-changes2?expand=1.
--
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 a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1170066848
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java:
##########
@@ -58,33 +61,81 @@ boolean isValid() {
return isValid;
}
+ /**
+ * Used to indicate if a user of this interface needs a tablet hosted or not. This simple enum was
+ * created instead of using a boolean for code clarity.
+ */
+ public enum HostingNeed {
+ HOSTED, NONE
+ }
+
+ // ELASTICITY_TODO rename to findTablet
+ /**
+ * Finds the tablet that contains the given row.
+ *
+ * @param hostingNeed When {@link HostingNeed#HOSTED} is passed will only return a tablet if it
+ * has location. When {@link HostingNeed#NONE} is passed will return the tablet that
+ * overlaps the row.
+ *
+ * @return overlapping tablet. If no overlapping tablet exists, returns null. If hosting is
+ * required and the tablet currently has no location ,returns null.
+ */
public abstract TabletLocation locateTablet(ClientContext context, Text row, boolean skipRow,
- boolean retry) throws AccumuloException, AccumuloSecurityException, TableNotFoundException;
+ HostingNeed hostingNeed)
+ throws AccumuloException, AccumuloSecurityException, TableNotFoundException;
+
+ public TabletLocation locateTabletWithRetry(ClientContext context, Text row, boolean skipRow,
Review Comment:
I'm curious about the use case where `HostingNeed.HOSTED` is passed to `locateTablet` and the Tablet is not hosted and the method returns null. To me, `locateTabletWithRetry` should not be necessary. `locateTablet` should internally retry when `HOSTED` is specified.
--
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] keith-turner commented on a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1170644151
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java:
##########
@@ -58,33 +61,81 @@ boolean isValid() {
return isValid;
}
+ /**
+ * Used to indicate if a user of this interface needs a tablet hosted or not. This simple enum was
+ * created instead of using a boolean for code clarity.
+ */
+ public enum HostingNeed {
Review Comment:
The majority of the code was passing false for retry and the retry logic complicated the TabletLocator, so I decided to scrap while making these changes. Alot of code was doing its own retry thing when getting null back, so I made the code return null in the case where HostingNeed was HOSTED and the tablet currently had no location. Returning null worked with existing code, we could have a follow on PR to improve on that pattern in the code. So its not replacing the retry variable in any functional way, it just happens to take its slot.
> I wonder if values REQUIRED and NOT_REQUIRED might make it more clear.
That does seem better, also thinking that renaming the enum type may help. Will make an update with the following.
```java
public enum LocationNeed {
REQUIRED, NOT_REQUIRED
}
```
--
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 a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1170179504
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ScanServerSelector.java:
##########
@@ -106,11 +108,46 @@ interface SelectorParameters {
* were set, an empty map is returned.
*/
Map<String,String> getHints();
+
+ /**
+ * This function helps a scan server selector wait for an optional to become non-empty (like
+ * waiting for scan servers to be present) and throws exceptions when waiting is no longer
+ * possible OR returning false if the max wait time was exceeded. The passed in condition will
+ * be periodically called and as long as it returns an empty optional the function will continue
+ * to wait.
+ *
+ * @param condition periodically calls this to see if it is non-empty.
+ * @param maxWaitTime the maximum time to wait for the condition to become non-empty
+ * @param description a description of what is being waited on, used for error messages and
+ * logging
+ * @return The first non-empty optional returned by the condition. An empty optional if the
+ * maxWaitTime was exceeded without the condition ever returning a non-empty optional.
+ *
+ * @throws org.apache.accumulo.core.client.TableDeletedException if the table is deleted while
+ * waiting for the condition to become non-empty. Do not catch this exception, let it
+ * escape.
+ * @throws org.apache.accumulo.core.client.TimedOutException if the timeout specified by
+ * {@link ScannerBase#setTimeout(long, TimeUnit)} is exceeded while waiting. Do not
+ * catch this exception, let it escape.
+ *
+ * @since ELASTICITY_TODO
Review Comment:
I think you can change these to 4.0.0. I merged the PR into elasticity branch that set this version.
--
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] keith-turner commented on a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1170635870
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/RootTabletLocator.java:
##########
@@ -57,19 +59,23 @@ public <T extends Mutation> void binMutations(ClientContext context, List<T> mut
Map<String,TabletServerMutations<T>> binnedMutations, List<T> failures) {
TabletLocation rootTabletLocation = getRootTabletLocation(context);
if (rootTabletLocation != null) {
- var tsm = new TabletServerMutations<T>(rootTabletLocation.getTserverSession());
+ var tsm = new TabletServerMutations<T>(rootTabletLocation.getTserverSession().get());
for (T mutation : mutations) {
tsm.addMutation(RootTable.EXTENT, mutation);
}
- binnedMutations.put(rootTabletLocation.getTserverLocation(), tsm);
+ binnedMutations.put(rootTabletLocation.getTserverLocation().get(), tsm);
} else {
failures.addAll(mutations);
}
}
@Override
public List<Range> locateTablets(ClientContext context, List<Range> ranges,
- BiConsumer<TabletLocation,Range> rangeConsumer) {
+ BiConsumer<TabletLocation,Range> rangeConsumer, HostingNeed hostingNeed) {
+
+ // only expect the hosted case so this code only handles that, so throw an exception is
+ // something else is seed
Review Comment:
Yeah, was only expecting request for the root tablet to have a location.
--
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 a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1170048948
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java:
##########
@@ -58,33 +61,81 @@ boolean isValid() {
return isValid;
}
+ /**
+ * Used to indicate if a user of this interface needs a tablet hosted or not. This simple enum was
+ * created instead of using a boolean for code clarity.
+ */
+ public enum HostingNeed {
Review Comment:
I have been having trouble wrapping my head around this enum. It seems to be taking place of the `retry` boolean. I wonder if values `REQUIRED` and `NOT_REQUIRED` might make it more clear.
--
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] keith-turner commented on a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1172004877
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -299,10 +305,52 @@ public ScanState(ClientContext context, TableId tableId, Authorizations authoriz
}
}
- public static class ScanTimedOutException extends IOException {
+ static <T> Optional<T> waitUntil(Supplier<Optional<T>> condition, Duration maxWaitTime,
Review Comment:
This method has some things very specific to scans in it so it could not be moved as is. However it could build on a more generic method and pass the scan specific code in as a lambda.
We may benefit from by combining functionality in the Retry and Wait classes to create a new class that can implement retry more succinctly throughout the code base.
--
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] DomGarguilo commented on a diff in pull request #3292: fixes #3252 support tablets w/ and w/o location in tablet location cache
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3292:
URL: https://github.com/apache/accumulo/pull/3292#discussion_r1171810234
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -299,10 +305,52 @@ public ScanState(ClientContext context, TableId tableId, Authorizations authoriz
}
}
- public static class ScanTimedOutException extends IOException {
+ static <T> Optional<T> waitUntil(Supplier<Optional<T>> condition, Duration maxWaitTime,
Review Comment:
It does seem like there could be some consolidation with these however I'm not sure what the best way to consolidate them would be.
--
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