-
Notifications
You must be signed in to change notification settings - Fork 751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GOBBLIN-1766] Define metric to measure lag from producing to consume… #3625
Conversation
… change stream events
@ZihanLi58 @phet can you review? |
Codecov Report
@@ Coverage Diff @@
## master #3625 +/- ##
============================================
+ Coverage 46.44% 46.60% +0.15%
- Complexity 10621 10667 +46
============================================
Files 2130 2133 +3
Lines 83303 83503 +200
Branches 9282 9281 -1
============================================
+ Hits 38692 38914 +222
+ Misses 41039 41030 -9
+ Partials 3572 3559 -13
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
gobblin-metrics-libs/gobblin-metrics-base/src/main/avro/GenericStoreChangeEvent.avsc
Show resolved
Hide resolved
...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree to have you confirm whether the txId is in fact what we want to key/cache on... otherwise leave a clarifying comment
gobblin-metrics-libs/gobblin-metrics-base/src/main/avro/GenericStoreChangeEvent.avsc
Outdated
Show resolved
Hide resolved
...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java
Outdated
Show resolved
Hide resolved
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/SpecStoreChangeMonitor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clearly took a closer read this time and have a few more Qs... sorry to have not noticed them earlier
gobblin-metrics-libs/gobblin-metrics-base/src/main/avro/GenericStoreChangeEvent.avsc
Show resolved
Hide resolved
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/kafka/HighLevelConsumer.java
Outdated
Show resolved
Hide resolved
gobblin-metrics-libs/gobblin-metrics-base/src/main/avro/GenericStoreChangeEvent.avsc
Outdated
Show resolved
Hide resolved
...modules/gobblin-kafka-09/src/test/java/org/apache/gobblin/runtime/HighLevelConsumerTest.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java
Outdated
Show resolved
Hide resolved
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/metrics/RuntimeMetrics.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems very close!
do let me know if you find deeper rationale to prove uniform partition delay. intuitively I don't believe that can be assumed, but possibly there's a critical factor I'm not aware of
gobblin-metrics-libs/gobblin-metrics-base/src/main/avro/GenericStoreChangeEvent.avsc
Show resolved
Hide resolved
...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java
Show resolved
Hide resolved
...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks good... will let you decide how to proceed on the measurement of non-uniform delay and whether anticipated to be an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
* upstream/master: [GOBBLIN-1771] Clean up logs for dataset commit and file cleanup (apache#3631) [GOBBLIN-1770] Allow null values for fields in GaaSObservabilityEvent.Issue fields which are optional [GOBBLIN-1769] Change a noisy log that indicates that the queue capacity is almost full to debug (apache#3629) [GOBBLIN-1766] Define metric to measure lag from producing to consume… (apache#3625) [GOBBLIN-1765] Add support to sync metadata for dir in manifest based copy (apache#3624) [GOBBLIN-1768] Fix constructor in KafkaJobStatusMonitorFactory so that it can be injected (apache#3628) Specifically name each Hikari connection pool created, for traceability (apache#3627)
… change stream events
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
Description
Modify event for change stream events to add the transaction identifier and produceTimestamp. Use the transaction identifier instead of timestamp to dedup events as "at least once" delivery may result in multiple change stream events with different timestamps for the same row update in mysql. Instead use the timestamp to emit a metric measuring the time between producing the event and consuming on our monitor.
Tests
Commits