You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/03/17 05:42:34 UTC
[GitHub] [incubator-gobblin] sv2000 opened a new pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
sv2000 opened a new pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928
… Kafka pipeline
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [x] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1087
### Description
- [x] Here are some details about my PR, including screenshots (if applicable):
In this PR, we instrument the KafkaExtractor to track the observed latency of Kafka consumer records processed by the pipeline. Here, observed latency is measured as the time difference between processing time of the record and the original creation time. The latency distribution is tracked in an HdrHistogram, which is serialized into a string when emitted as part of a GobblinTrackingEvent.
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Added unit tests and a JMH benchmarking test.
### Commits
- [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] codecov-io commented on issue #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#issuecomment-599897179
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=h1) Report
> Merging [#2928](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/ae7dad01599a255da05f4a44fe1a0093c75446fa?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2928 +/- ##
===========================================
- Coverage 4.12% 4.12% -0.01%
Complexity 750 750
===========================================
Files 1936 1936
Lines 73138 73188 +50
Branches 8071 8080 +9
===========================================
Hits 3016 3016
- Misses 69802 69852 +50
Partials 320 320
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ache/gobblin/kafka/client/KafkaConsumerRecord.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2thZmthL2NsaWVudC9LYWZrYUNvbnN1bWVyUmVjb3JkLmphdmE=) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...source/extractor/extract/kafka/KafkaExtractor.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYUV4dHJhY3Rvci5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...ctor/extract/kafka/KafkaExtractorStatsTracker.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYUV4dHJhY3RvclN0YXRzVHJhY2tlci5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...in/source/extractor/extract/kafka/KafkaSource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYVNvdXJjZS5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=footer). Last update [ae7dad0...b9ca6d1](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] asfgit closed pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#discussion_r393982314
##########
File path: gobblin-modules/gobblin-kafka-common/src/main/java/org/apache/gobblin/source/extractor/extract/kafka/KafkaExtractorStatsTracker.java
##########
@@ -161,14 +208,24 @@ public void resetStartFetchEpochTime(int partitionIdx) {
* @param decodeStartTime the time instant immediately before a record decoding begins.
* @param recordSizeInBytes the size of the decoded record in bytes.
* @param logAppendTimestamp the log append time of the {@link org.apache.gobblin.kafka.client.KafkaConsumerRecord}.
+ * @param recordCreationTimestamp the time of the {@link org.apache.gobblin.kafka.client.KafkaConsumerRecord}.
*/
- public void onDecodeableRecord(int partitionIdx, long readStartTime, long decodeStartTime, long recordSizeInBytes, long logAppendTimestamp) {
+ public void onDecodeableRecord(int partitionIdx, long readStartTime, long decodeStartTime, long recordSizeInBytes, long logAppendTimestamp, long recordCreationTimestamp) {
this.statsMap.computeIfPresent(this.partitions.get(partitionIdx), (k, v) -> {
long currentTime = System.nanoTime();
v.processedRecordCount++;
v.partitionTotalSize += recordSizeInBytes;
v.decodeRecordTime += currentTime - decodeStartTime;
v.readRecordTime += currentTime - readStartTime;
+ if (this.observedLagHistogram != null && recordCreationTimestamp > 0) {
+ long observedLag = System.currentTimeMillis() - recordCreationTimestamp;
Review comment:
Shall we use "latency" instead since "lag" internally refers to number of events behind the head of stream ? Maybe you can come up with better words, it is just my intuition of "lag" refers to something else.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#discussion_r393980057
##########
File path: gobblin-modules/gobblin-kafka-common/src/main/java/org/apache/gobblin/source/extractor/extract/kafka/KafkaExtractorStatsTracker.java
##########
@@ -398,5 +480,8 @@ public void reset() {
for (int partitionIdx = 0; partitionIdx < this.partitions.size(); partitionIdx++) {
resetStartFetchEpochTime(partitionIdx);
}
+ if (this.observedLagHistogram != null) {
+ this.observedLagHistogram.reset();
Review comment:
+1
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] sv2000 commented on issue #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
sv2000 commented on issue #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#issuecomment-599888455
@autumnust @ZihanLi58 please review.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] codecov-io edited a comment on issue #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#issuecomment-599897179
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=h1) Report
> Merging [#2928](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/ae7dad01599a255da05f4a44fe1a0093c75446fa?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2928 +/- ##
===========================================
- Coverage 4.12% 4.12% -0.01%
Complexity 750 750
===========================================
Files 1936 1936
Lines 73138 73188 +50
Branches 8071 8080 +9
===========================================
Hits 3016 3016
- Misses 69802 69852 +50
Partials 320 320
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ache/gobblin/kafka/client/KafkaConsumerRecord.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2thZmthL2NsaWVudC9LYWZrYUNvbnN1bWVyUmVjb3JkLmphdmE=) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...source/extractor/extract/kafka/KafkaExtractor.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYUV4dHJhY3Rvci5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...ctor/extract/kafka/KafkaExtractorStatsTracker.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYUV4dHJhY3RvclN0YXRzVHJhY2tlci5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...in/source/extractor/extract/kafka/KafkaSource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYVNvdXJjZS5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=footer). Last update [ae7dad0...00ce195](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#discussion_r393760770
##########
File path: gobblin-modules/gobblin-kafka-common/src/main/java/org/apache/gobblin/source/extractor/extract/kafka/KafkaExtractorStatsTracker.java
##########
@@ -398,5 +480,8 @@ public void reset() {
for (int partitionIdx = 0; partitionIdx < this.partitions.size(); partitionIdx++) {
resetStartFetchEpochTime(partitionIdx);
}
+ if (this.observedLagHistogram != null) {
+ this.observedLagHistogram.reset();
Review comment:
Is the `reset` method reclaim memory ? Would it be safer if destroy the object and re-create next time if reclaim is not happening ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] codecov-io edited a comment on issue #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#issuecomment-599897179
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=h1) Report
> Merging [#2928](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/ae7dad01599a255da05f4a44fe1a0093c75446fa&el=desc) will **increase** coverage by `40.38%`.
> The diff coverage is `45.45%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2928 +/- ##
=============================================
+ Coverage 4.12% 44.51% +40.38%
- Complexity 750 8946 +8196
=============================================
Files 1936 1936
Lines 73138 73188 +50
Branches 8071 8080 +9
=============================================
+ Hits 3016 32576 +29560
+ Misses 69802 37577 -32225
- Partials 320 3035 +2715
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ache/gobblin/kafka/client/KafkaConsumerRecord.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2thZmthL2NsaWVudC9LYWZrYUNvbnN1bWVyUmVjb3JkLmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...source/extractor/extract/kafka/KafkaExtractor.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYUV4dHJhY3Rvci5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...in/source/extractor/extract/kafka/KafkaSource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYVNvdXJjZS5qYXZh) | `1.34% <23.07%> (+1.34%)` | `1.00 <0.00> (+1.00)` | |
| [...ctor/extract/kafka/KafkaExtractorStatsTracker.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYUV4dHJhY3RvclN0YXRzVHJhY2tlci5qYXZh) | `80.90% <59.45%> (+80.90%)` | `35.00 <6.00> (+35.00)` | |
| [...gobblin/runtime/mapreduce/GobblinOutputFormat.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbWFwcmVkdWNlL0dvYmJsaW5PdXRwdXRGb3JtYXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [...askStateCollectorServiceHiveRegHandlerFactory.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvVGFza1N0YXRlQ29sbGVjdG9yU2VydmljZUhpdmVSZWdIYW5kbGVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [...re/filesystem/FsDatasetStateStoreEntryManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbWV0YXN0b3JlL2ZpbGVzeXN0ZW0vRnNEYXRhc2V0U3RhdGVTdG9yZUVudHJ5TWFuYWdlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...in/runtime/mapreduce/CustomizedProgresserBase.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbWFwcmVkdWNlL0N1c3RvbWl6ZWRQcm9ncmVzc2VyQmFzZS5qYXZh) | `0.00% <0.00%> (-83.34%)` | `0.00% <0.00%> (-1.00%)` | |
| [...rg/apache/gobblin/runtime/ZkDatasetStateStore.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4taGVsaXgvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vcnVudGltZS9aa0RhdGFzZXRTdGF0ZVN0b3JlLmphdmE=) | `0.00% <0.00%> (-80.77%)` | `0.00% <0.00%> (-7.00%)` | |
| [...lin/runtime/locks/LegacyJobLockFactoryManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbG9ja3MvTGVnYWN5Sm9iTG9ja0ZhY3RvcnlNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-78.58%)` | `0.00% <0.00%> (-2.00%)` | |
| ... and [1143 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2928/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=footer). Last update [ae7dad0...14ac700](https://codecov.io/gh/apache/incubator-gobblin/pull/2928?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#discussion_r393837517
##########
File path: gobblin-modules/gobblin-kafka-common/src/main/java/org/apache/gobblin/source/extractor/extract/kafka/KafkaExtractorStatsTracker.java
##########
@@ -398,5 +480,8 @@ public void reset() {
for (int partitionIdx = 0; partitionIdx < this.partitions.size(); partitionIdx++) {
resetStartFetchEpochTime(partitionIdx);
}
+ if (this.observedLagHistogram != null) {
+ this.observedLagHistogram.reset();
Review comment:
Added benchmark to compare reset vs a new Histogram creation. While both reset and new are cheap, reset is 3x cheaper than new object creation. reset simply keeps the allocated count array as is and zeroes out the array. In general, it would be better to avoid new object creation to avoid GCs/memory fragmentation that can occur over time.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] shirshanka commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
shirshanka commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#discussion_r393964368
##########
File path: gobblin-modules/gobblin-kafka-common/build.gradle
##########
@@ -44,6 +46,9 @@ dependencies {
testCompile project(":gobblin-test-utils")
testCompile externalDependency.mockito
testCompile externalDependency.testng
+
+ jmh 'org.openjdk.jmh:jmh-core:1.17.3'
Review comment:
Now that we seem to have multiple modules using jmh, can you move out the version definition to common place like definitions.gradle and refer to it like we refer to them normally?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#discussion_r393835783
##########
File path: gobblin-modules/gobblin-kafka-common/src/jmh/java/org/apache/gobblin/source/extractor/extract/kafka/HdrHistogramPerformanceBenchmark.java
##########
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.source.extractor.extract.kafka;
+
+import java.util.concurrent.TimeUnit;
+import java.util.stream.IntStream;
+
+import org.HdrHistogram.Histogram;
+import org.apache.commons.math3.random.RandomDataGenerator;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.runner.Runner;
+import org.openjdk.jmh.runner.options.ChainedOptionsBuilder;
+import org.openjdk.jmh.runner.options.OptionsBuilder;
+
+import lombok.extern.slf4j.Slf4j;
+
+
+/**
+ * A micro-benchmark to measure the time taken to serialize a {@link Histogram} instance to its String representation. The
+ * benchmark uses a Random number generator to generate values according to a Uniform Distribution, an adversarial pattern
+ * for a Histogram that is likely to produce more count buckets in comparison with a skewed distribution. The benchmark
+ * provides an upper bound on memory footprint of the histogram, serialization time, as well as the size of the
+ * serialized representation.
+ */
+@Warmup (iterations = 3)
+@Measurement (iterations = 10)
+@BenchmarkMode (value = Mode.AverageTime)
+@Fork (value = 1)
+@OutputTimeUnit (TimeUnit.MILLISECONDS)
+@Slf4j
+public class HdrHistogramPerformanceBenchmark {
+
+ @State (value = Scope.Benchmark)
+ public static class HistogramState {
+ private static long MIN_VALUE = 1;
+ private static long MAX_VALUE = TimeUnit.HOURS.toMillis(24);
+
+ private Histogram histogram1;
+ private Histogram histogram2;
+ private Histogram histogram3;
+ private Histogram histogram4;
+
+ private final RandomDataGenerator random = new RandomDataGenerator();
+
+ @Setup (value = Level.Iteration)
+ public void setUp() {
+ this.histogram1 = buildHistogram(1000000);
+ this.histogram2 = buildHistogram(2000000);
+ this.histogram3 = buildHistogram(4000000);
+ this.histogram4 = buildHistogram(10000000);
Review comment:
There are 2 ways to address this question:
1. The Histogram performance itself (e.g. adding a value or memory size) are independent of number of samples. They are only dependent on the range of values and the precision needed to report a value. The benchmark numbers indeed confirm that the size is indeed constant across different counts and is around 150 KB. The benchmark also confirms that the serialization constants are also more or less independent of number of samples observed.
2. Assuming a typical container processing rates in the order of 10-20 MB/s. Assuming records of size 1K, which corresponds to 10-20K records/second, a histogram should hold counts for 3-6M records within a flush interval of 5 minutes.
I have updated the description of this PR to include results from running the benchmark on a Macbook.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #2928: GOBBLIN-1087: Track and report histogram of observed lag from Gobblin…
URL: https://github.com/apache/incubator-gobblin/pull/2928#discussion_r393756273
##########
File path: gobblin-modules/gobblin-kafka-common/src/jmh/java/org/apache/gobblin/source/extractor/extract/kafka/HdrHistogramPerformanceBenchmark.java
##########
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.source.extractor.extract.kafka;
+
+import java.util.concurrent.TimeUnit;
+import java.util.stream.IntStream;
+
+import org.HdrHistogram.Histogram;
+import org.apache.commons.math3.random.RandomDataGenerator;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.runner.Runner;
+import org.openjdk.jmh.runner.options.ChainedOptionsBuilder;
+import org.openjdk.jmh.runner.options.OptionsBuilder;
+
+import lombok.extern.slf4j.Slf4j;
+
+
+/**
+ * A micro-benchmark to measure the time taken to serialize a {@link Histogram} instance to its String representation. The
+ * benchmark uses a Random number generator to generate values according to a Uniform Distribution, an adversarial pattern
+ * for a Histogram that is likely to produce more count buckets in comparison with a skewed distribution. The benchmark
+ * provides an upper bound on memory footprint of the histogram, serialization time, as well as the size of the
+ * serialized representation.
+ */
+@Warmup (iterations = 3)
+@Measurement (iterations = 10)
+@BenchmarkMode (value = Mode.AverageTime)
+@Fork (value = 1)
+@OutputTimeUnit (TimeUnit.MILLISECONDS)
+@Slf4j
+public class HdrHistogramPerformanceBenchmark {
+
+ @State (value = Scope.Benchmark)
+ public static class HistogramState {
+ private static long MIN_VALUE = 1;
+ private static long MAX_VALUE = TimeUnit.HOURS.toMillis(24);
+
+ private Histogram histogram1;
+ private Histogram histogram2;
+ private Histogram histogram3;
+ private Histogram histogram4;
+
+ private final RandomDataGenerator random = new RandomDataGenerator();
+
+ @Setup (value = Level.Iteration)
+ public void setUp() {
+ this.histogram1 = buildHistogram(1000000);
+ this.histogram2 = buildHistogram(2000000);
+ this.histogram3 = buildHistogram(4000000);
+ this.histogram4 = buildHistogram(10000000);
Review comment:
Within our flush time-window, what's the magnitude of #events? Curious on the number ensuring that we are indeed doing stress test here.
----------------------------------------------------------------
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
With regards,
Apache Git Services