HADOOP-17272. ABFS Streams to support IOStatistics API#2604
Conversation
This comment has been minimized.
This comment has been minimized.
steveloughran
left a comment
There was a problem hiding this comment.
LGTM; some minor comments, and checkstyle & javadocs need fixing.
|
Checkstyle issues are due to static imports having ".*", but these are enums, should I go this route or add the class name before their name(adds a few bits in the code, but avoids checkstyle issue)? Edit: Just saw some indentation issues as well, which my maven checkstyle didn't find. I'll update those in the new commit along with the change in static import if needed) |
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
| import java.util.concurrent.Future; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import org.apache.hadoop.fs.statistics.StreamStatisticNames; |
There was a problem hiding this comment.
nit: needs to go into the real org.apache block. The IDE is getting confused because of the third party stuff. as this is a "real" hadoop import it should go down below; when we backport the google ones will go back to their unshaded names
There was a problem hiding this comment.
Sorry, my bad, should've double-checked.
|
|
||
| import java.util.concurrent.atomic.AtomicLong; | ||
|
|
||
| import org.apache.hadoop.fs.statistics.StreamStatisticNames; |
There was a problem hiding this comment.
move into real apache group.
steveloughran
left a comment
There was a problem hiding this comment.
LGTM, just some import positioning. The move to shaded guava on trunk is complicating both imports and backporting, even though it will ultimately be good
|
+1 pending those imports |
|
🎊 +1 overall
This message was automatically generated. |
Contributed by Mehakmeet Singh. Change-Id: I3445dec84b9b9e43bb1e41f709944ea05416bd74
Previously #2353, Had to close it due to parent branch closing and pushed in a new PR. Now the IOStatistics has been merged in trunk as 3 PRs(#2577, #2579, #2580).
Tested using: mvn -T 1C -Dparallel-tests=abfs clean verify
Region: East US
[errors related to #2331]