[GitHub] [accumulo] ctubbsii commented on a change in pull request #2160: Attempt to fix problems with cacheId

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #2160:
URL: https://github.com/apache/accumulo/pull/2160#discussion_r651002414



##########
File path: core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java
##########
@@ -347,7 +347,7 @@ public SamplerConfiguration getSamplerConfiguration() {
       for (int i = 0; i < sources.length; i++) {
         // TODO may have been a bug with multiple files and caching in older version...
         FSDataInputStream inputStream = (FSDataInputStream) sources[i].getInputStream();
-        CachableBuilder cb = new CachableBuilder().cacheId("source-" + i).input(inputStream)
+        CachableBuilder cb = new CachableBuilder().cacheId(opts.in.getPaths()[i]).input(inputStream)

Review comment:
       While this is more informative than `source-1,source-2,...`, I'm not sure this will work in all cases. The Opts are constructed using RFileScannerBuilder from either a set of RFileSource objects, or a set of String filenames. We only have the path information if the builder used the String filenames. But, the RFileSource version only has InputStreams.
   
   The `getSources()` method will translate any provided String filenames into RFileSources, but any that were passed in as RFileSource won't materialize path information in the reverse, in your new `getPaths()` method. Rather, that new method you added will only show paths if it was constructed with paths. It won't show anything that was constructed with RFileSources.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org