DomGarguilo commented on code in PR #3980:
URL: https://github.com/apache/accumulo/pull/3980#discussion_r1408234945
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/ConcurrentKeyExtentCache.java:
##########
@@ -88,9 +88,10 @@ protected void updateCache(KeyExtent e) {
@VisibleForTesting
protected Stream<KeyExtent> lookupExtents(Text row) {
- return TabletsMetadata.builder(ctx).forTable(tableId).overlapping(row,
true, null)
- .checkConsistency().fetch(PREV_ROW).build().stream().limit(100)
- .map(TabletMetadata::getExtent);
+ try (TabletsMetadata tabletsMetadata =
TabletsMetadata.builder(ctx).forTable(tableId)
+ .overlapping(row, true,
null).checkConsistency().fetch(PREV_ROW).build()) {
+ return
tabletsMetadata.stream().limit(100).map(TabletMetadata::getExtent);
Review Comment:
I ended up making changes to this case along the same lines as your
suggestion but am thinking that passively relying on other code to close the
stream seems fragile and there is no guarantee that it will happen. I am
looking into these cases and trying to see if they can be refactored to return
a TabletsMetadata object directly so that the calling code is more likely to
deal with closing the resource but is seems like lots of places are going to
have to use the `onClose()`.
This is just an observation but seems like a good argument against patterns
that do not enforce closing of resources like this (like returning a stream or
iterator created from a `TabletsMetadata` object)
--
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]
