HADOOP-16830. IOStatistics API. #1982
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Successor to #1820. regarding AWS metric failures, when enabled code which goes near landat and common crawl buckets are failing, even when the default endpoint is being used |
| * This package contains support for statistic collection and reporting. | ||
| * This is the public API; implementation classes are to be kept elsewhere. | ||
| * | ||
| * This package is defines two interfaces |
mehakmeet
reviewed
May 3, 2020
| * @param eval evaluator for the statistic | ||
| * @return the builder. | ||
| */ | ||
| public DynamicIOStatisticsBuilder add(String key, |
There was a problem hiding this comment.
How can I add a counter of type long through the builder?
There was a problem hiding this comment.
via a lambda expression
mehakmeet
reviewed
May 3, 2020
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInstrumentation.java
Outdated
Show resolved
Hide resolved
mehakmeet
reviewed
May 3, 2020
| LOG.info("Statistics = {}", strVal); | ||
| verifyStatisticValue(statistics, STREAM_WRITE_BYTES, 1); | ||
| } finally { | ||
| fs.delete(path, false); |
There was a problem hiding this comment.
Is fs.delete() required in these tests? Won't teardown() take care of it?
There was a problem hiding this comment.
just being thorough
mehakmeet
reviewed
May 3, 2020
| } | ||
|
|
||
| /** | ||
| * Keys which the output stream must support. |
it takes any lambda expression, so you can go add(key, (key) ->this.getLongval()). Having some method add(key, long) wouldn't work, as that's not an evaluator, only a static value. |
|
failure on a test run, which is related to stats gathering. But I don't see this when run from the IDE. Race condition? |
|
cannot repeat this was an eight thread parallel run, maybe there was some retry/timeout |
|
💔 -1 overall
This message was automatically generated. |
|
|
add methods to add two iostatistics instances, and to subtract one from the other. This will help merging stats from multiple file read/writes, and for when we add the API to a long-life instance (e.g the s3a and abfs connectors), we can isolate IO better (though not across threads) |
|
This patch adds LocalFS/RawLocalFS/ChecksummedFileSystem statistics passthrough and collection. Getting everything passed through is actually the harder part of the process... The fact that the raw local streams are buffered complicates testing. Although it makes for a bigger patch, it means that we get unit tests in new interface/impl to make it easy to instrument a class; which can then be set or incremented |
|
💔 -1 overall
This message was automatically generated. |
Contributed by Steve Loughran. This patch adds to hadoop-common an API For querying IO classes (Especially input and outpu streams) for statistics. It includes a big rework of the S3A Statistics including * implementation of the IOStatistics APIs * and contract tests for those and any other streams which implement the same interfaces and the same bytes read/written counters. * A split of the existing S3AInstrumentation classes into interface/implementations. * Troubled attempt to wire up the AWSSDK metrics The AWS metri binding will need to be split out and addressed separately because the wiring up is breaking some of our region handling code Doing the public interface hand-in-hand with that implementation helps evolve the interface, but it makes for a bigger patch. and contract tests for those and any other streams which implement the same interfaces and the same bytes read/written counters. Proposed: once the reviewers are happy with the design we can split the two up into the hadoop-common changes (which can be used in ABFS) and the S3A FS changes. Writing up the package info class makes me think it is a bit overcomplicated right now and that maybe we should go for "dynamic always" statistics. If you want a snapshot, it can be wrapped with IOStatisticsSupport.takeSnapshot(). This will simplify the client code and remove ambiguity in implementations as to what they should be doing. We either provide callbacks to evaluate values or references to AtomicLongs/AtomicIntegers which are probed on demand. Change-Id: I91ae225d7def59602e84729f62a32f77158d97f6
This retains all the logic for using the builder API To create the AWS SDK S3 client -but as that was failing in our endpoint/region setup logic, it's been disabled in failing tests and in production. it's still there ready to be turned on -once someone fixes the regression. Change-Id: If3251b7c835ad7da73bc2666cfa743d68f19ed24
…ounters aren't updating correctly (or wrong ones...) Change-Id: Ic2ca78fa5694dd579394c17fb01879cad6ebe8e1
Drastically simplify statistics API by removing any distinction between snapshot/dynamic; everything is expected to be dynamic. There is a way to snapshot them, and that is actually serializable (Tested). This allows things like Spark to serialize statistics without extra work. Add explicit tests for the DynamicIOStatistics which is expected to be the sole implementation applications are likely to need. This should be fairly straightforward to work with; IOStatisticsLogging is the public API to convert statistics to strings robustly, especially in log statements. AWS tests are failing with * the broken region stuff * the put request count isn't being updated; ITestS3AHugeMagicCommits>AbstractSTestS3AHugeFiles.test_010_CreateHugeFile is failing. Change-Id: I4d14cbef3a24379828b10ddc6a4cd793cb5d6f73
* Tune IOStatisticsLogging methods and output. * Review/improve javadocs * turn off failing network binding tests related to endpoint selection. * Reinstate direct update of StorageStatistics values in S3AFS.IncrementStatistics The last one was me thinking that the statistics context was back-updating the filesystem storage statistics. IMO, it should; for now I just reinstated the method. Change-Id: I34033dd04a9cf88f84e2afd88eb70fdd127c5192
* Address Mukund's comments * Add markdown page on the API * Add tests for the on-demand stringifier * Make S3A BlockOutputStreamStatistics and S3AInputStreamStatistics implement IOStatisticsSource and provide their (on-demand) IOStatistics instances through this API. That is: use it internally as well as a public API. Change-Id: I42b8a17b2cf9af03ca358d4e0ac3ebddf71d12f0
Change-Id: I6aa053bd713dc754bb4428f075dbabae4466914c cleanup: checkstyle, javadoc and other tweaks
This patch adds LocalFS/RawLocalFS/ChecksummedFileSystem statistics passthrough and collection. Getting everything passed through is actually the harder part of the process... I ended up having to debug things just work out what is going on there. The fact that the raw local streams are buffered complicates testing. The write tests expect the counter to not update until the stream has closed; I need to expand the read tests this way too. Although it makes for a bigger patch, it means that we get unit tests in hadoop-common and that passthrough is all correct. It will also permit applications to collect IO statistics on local storage operations. Change-Id: Ibf9ebdb55aa57ef95199d5ccb6783cbf10216db9
Change-Id: I64e5c0e91d05e245e0615e695fb32f8a83f61958
steveloughran
force-pushed
the
s3/HADOOP-16830-iostatistics
branch
from
72fe39e to
395ecd5
Compare
|
💔 -1 overall
This message was automatically generated. |
+javadocs Change-Id: I83506eacf2fdec80e0db3c5fe23fb39b61b16abd
|
💔 -1 overall
This message was automatically generated. |
handoff ~all counters to CounterIOStatistics; extending that for the expanded use (add, diff) We could actually simplify the inputstats interface by just using get/set on the keys -avoiding this for a bit of future flexibility Change-Id: I649e8c5a8e8571b032a5a14590bb71150db1c95b
|
💔 -1 overall
This message was automatically generated. |
Change-Id: I2b6aa6b1dd3ac5844f372906228b3b8dfc39c38e
Change-Id: Ife60eabcfe78c54d2e15ad5d7365d1a42ad48ed9
|
💔 -1 overall
This message was automatically generated. |
|
checkstyle. I intend to ignore those about _1, _2 and _3 methods as they match scala's; I plan to soon add tuple/triple classes with these to hadoop utils |
Change-Id: I10497244526a133091babcf45bf24af6f8d8c3d6
|
💔 -1 overall
This message was automatically generated. |
|
aah, i need to rebase. @mehakmeet -this is going to complicate your life. sorry. I'll do a whole new PR. having written the new extensible design, I've decided I don't like it. It is too complex as I'm trying to support arbitrary arity tuples of any kind of statistic.it makes iterating/parsing this stuff way too complext here's a better idea: we only support a limited set;
what do people think? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributed by Steve Loughran.
This patch adds to hadoop-common an API For querying IO classes (Especially
input and output streams) for statistics.
It includes a big rework of the S3A Statistics including
The AWS metric binding is breaking some of the S3 region handling code, so is
disabled, we're still using the old "create client then set endpoint" logic rather
than the builder API for constructing the S3 client.
Doing the public interface hand-in-hand with that implementation helps evolve
the interface, but it makes for a bigger patch.
There are contract tests for those and any other streams which implement
the same interfaces and the same bytes read/written counters.
Proposed: once the reviewers are happy with the design we can split the two up
into the hadoop-common changes (which can be used in ABFS) and the S3A FS
changes.