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/27 01:12:58 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3349: improves fetching mulitple extents in ample
keith-turner opened a new pull request, #3349:
URL: https://github.com/apache/accumulo/pull/3349
Its possible to pass a collection of extents to ample and get the metadata for those tablets. However only metadata for the tablets that exists in the metadata are returned. Using this method correctly is tricky because it may in rare cases (like concurrent splits and merges) only return a subset of what was requested. This commit changes the ample interface to add handling for these missing extents by adding a Consumer<KeyExtent> to the interface to which missing extents are passed. Now its no longer possible to use this ample functionality without considering this rare case. While making this change one place was found where the missing extents were not being considered.
--
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 #3349: improves fetching mulitple extents in ample
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1181926800
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
I would like to to pass something that lets ample know they don't care about the missing extents, because the implementation could then stream directly from the scanner. When the caller wants to know what is missing ample has to buffer everything. Thought about passing in an Optional<Consumer<>>, then when a caller does not care they can pass in Optional.empty().
--
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 diff in pull request #3349: improves fetching mulitple extents in ample
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1185917428
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
I think I understand what you're saying now. My comment was only about readability (giving `e -> {}` a name). I now understand that you had entirely unrelated reasons for choosing Optional (to bypass doing unnecessary costly tasks). If I'm understanding correctly and the use of Optional allows you to bypass those costly tasks, then I think Optional makes sense. It just wasn't clear to me that the discussion had moved away from the readability point I was making.
--
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 #3349: improves fetching mulitple extents in ample
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1184377049
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
I am not trying to avoid the cost of creating a lambda. The implementation needs to know when the user does not care about the missing extents so it can take a much more efficient code path. With only a lambda there is no way to know if the consumer will discard everything its passed or not. That is why the implementation needs to differentiate between a lambda being passed and one not being passed, which is where the optional comes in.
--
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 diff in pull request #3349: improves fetching mulitple extents in ample
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1181917744
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
Seems like we use a NOOP consumer in a few places. Might be useful to assign that to a variable like `ignoreNotFound`
##########
server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionFinalizer.java:
##########
@@ -155,7 +155,7 @@ private void processPending() {
Map<KeyExtent,TabletMetadata> tabletsMetadata;
var extents = batch.stream().map(ExternalCompactionFinalState::getExtent).collect(toList());
- try (TabletsMetadata tablets = context.getAmple().readTablets().forTablets(extents)
+ try (TabletsMetadata tablets = context.getAmple().readTablets().forTablets(extents, e -> {})
Review Comment:
What's going on with this NOOP consumer?
--
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 #3349: improves fetching mulitple extents in ample
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1181926241
##########
server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionFinalizer.java:
##########
@@ -155,7 +155,7 @@ private void processPending() {
Map<KeyExtent,TabletMetadata> tabletsMetadata;
var extents = batch.stream().map(ExternalCompactionFinalState::getExtent).collect(toList());
- try (TabletsMetadata tablets = context.getAmple().readTablets().forTablets(extents)
+ try (TabletsMetadata tablets = context.getAmple().readTablets().forTablets(extents, e -> {})
Review Comment:
Thats for when the operation is ok with only getting a subset of what it asked for.
--
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 #3349: improves fetching mulitple extents in ample
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1181678658
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -792,6 +793,13 @@ public void run() {
MetadataUpdateCount counter = updateCounts.get(extent);
tablet.compareTabletInfo(counter, tabletMetadata);
}
+
+ for (var extent : missingTablets) {
+ Tablet tablet = onlineTabletsSnapshot.get(extent);
+ if (!tablet.isClosed()) {
+ log.error("Tablet {} is open but does not exists in metadata table.", extent);
Review Comment:
```suggestion
log.error("Tablet {} is open but does not exist in metadata table.", extent);
```
--
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 #3349: improves fetching mulitple extents in ample
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1181998939
##########
server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionFinalizer.java:
##########
@@ -155,7 +155,7 @@ private void processPending() {
Map<KeyExtent,TabletMetadata> tabletsMetadata;
var extents = batch.stream().map(ExternalCompactionFinalState::getExtent).collect(toList());
- try (TabletsMetadata tablets = context.getAmple().readTablets().forTablets(extents)
+ try (TabletsMetadata tablets = context.getAmple().readTablets().forTablets(extents, e -> {})
Review Comment:
Since there can be a performance advantage when the consumer is not needed, I think optional makes that intention very clear and the code can avoid trying to compute the missing extents.
--
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 #3349: improves fetching mulitple extents in ample
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1186412989
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
> If I'm understanding correctly and the use of Optional allows you to bypass those costly tasks, then I think Optional makes sense.
That's correct. There was javadoc covering this, but it was not very detailed. I added more details to the java doc in 958b868
--
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 diff in pull request #3349: improves fetching mulitple extents in ample
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1181963021
##########
server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionFinalizer.java:
##########
@@ -155,7 +155,7 @@ private void processPending() {
Map<KeyExtent,TabletMetadata> tabletsMetadata;
var extents = batch.stream().map(ExternalCompactionFinalState::getExtent).collect(toList());
- try (TabletsMetadata tablets = context.getAmple().readTablets().forTablets(extents)
+ try (TabletsMetadata tablets = context.getAmple().readTablets().forTablets(extents, e -> {})
Review Comment:
I made this comment before making my other comment. I think in the absence of a named lambda, a small inline comment explaining `e -> {}` would help. At a glance, it looks like a bug, because it looks like you're iterating over tablets, but not doing anything with them (because `forTablets` looks like a terminal operation like `forEach`), so this looks like `forEach(e -> { /* noop */})`. But naming the lambda as I suggested in my other comment is the most concise explanation that the NOOP consumer was chosen on purpose.
--
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 #3349: improves fetching mulitple extents in ample
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1184377049
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
I am not trying to avoid the cost of creating a lambda. The implementation needs to know when the user does not care about the missing extents so it can take a much more efficient code path. With only a lambda there is no way to know if the consumer will discard everything its passed or not. That is why the implementation needs to differentiate between a lambda being passed and one not being passed.
--
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 diff in pull request #3349: improves fetching mulitple extents in ample
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1183213029
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
I still think the named function is easier to understand, and doesn't require the unnecessary wrapping/unwrapping of a Consumer into an Optional. While not important for performance reasons, I just don't think the boilerplate wrapping/unwrapping is necessary or very clean. There is plenty of precedent for naming functions for reuse like this, either by assigning to a variable or by using a method reference. I just think it's cleaner, and makes the code much easier to read. But it's not that important to me (not enough to put in the effort to persuade, anyway), and I guess the Optional does achieve a similar purpose.
--
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 #3349: improves fetching mulitple extents in ample
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3349:
URL: https://github.com/apache/accumulo/pull/3349
--
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 diff in pull request #3349: improves fetching mulitple extents in ample
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1181961317
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
Yeah, I'm just suggesting giving it a name, so it's more clear, rather than have the lambda be anonymous.
For example:
```java
private static final Consumer<KeyExtent> IGNORE_MISSING = e -> {};
// ...
var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, IGNORE_MISSING).build()
```
--
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 #3349: improves fetching mulitple extents in ample
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1181998029
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
I made it optional in 0533f92. Its not used frequently so the object creation cost does not matter.
--
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 diff in pull request #3349: improves fetching mulitple extents in ample
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1183213029
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e -> {}).build()
Review Comment:
I still think the named function is easier to understand, and doesn't require the unnecessary wrapping/unwrapping of a Consumer into an Optional. While not important for performance reasons, I just don't think the boilerplate wrapping/unwrapping is necessary or very clean. There is plenty of precedent for naming functions for reuse like this, either by assigning to a variable or by using a method reference. I just think it's cleaner, and makes the code much easier to read. It's also [a pretty standard recommendation](https://stackoverflow.com/a/29851786). But it's not that important to me (not enough to put in the effort to persuade, anyway), and I guess the Optional does achieve a similar purpose.
--
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