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 2021/08/16 23:51:33 UTC
[GitHub] [gobblin] arjun4084346 opened a new pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
arjun4084346 opened a new pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367
…nd then returning its size
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-1518
### Description
- [x] Here are some details about my PR, including screenshots (if applicable):
During a memory profiling we noticed that a lot of CPU cycles are used in calling getSpecs().size() from metric reporting thread which runs in every n seconds. Right now there is no API to get size of the spec store.
Doing getAllSpecs and then calling size() over it, does a lot of unnecessary work viz reading the content of spec.
This PR will provide a method to direct get the size of the spec store without doing any unnecessary work.
Options
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### 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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter edited a comment on pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#issuecomment-899899812
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3367](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4193416) into [master](https://codecov.io/gh/apache/gobblin/commit/d44c12bb9e2e73156826de041fd4932da5b610f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d44c12b) will **increase** coverage by `0.00%`.
> The diff coverage is `64.28%`.
[](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #3367 +/- ##
=========================================
Coverage 46.53% 46.54%
- Complexity 10148 10155 +7
=========================================
Files 2055 2055
Lines 79894 79921 +27
Branches 8911 8913 +2
=========================================
+ Hits 37177 37196 +19
- Misses 39271 39278 +7
- Partials 3446 3447 +1
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/gobblin/runtime/api/SpecCatalog.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL1NwZWNDYXRhbG9nLmphdmE=) | `62.71% <0.00%> (ø)` | |
| [.../gobblin/runtime/spec\_catalog/TopologyCatalog.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19jYXRhbG9nL1RvcG9sb2d5Q2F0YWxvZy5qYXZh) | `54.08% <0.00%> (-1.71%)` | :arrow_down: |
| [...ache/gobblin/runtime/spec\_catalog/FlowCatalog.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19jYXRhbG9nL0Zsb3dDYXRhbG9nLmphdmE=) | `49.65% <33.33%> (-0.35%)` | :arrow_down: |
| [...che/gobblin/runtime/api/InstrumentedSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0luc3RydW1lbnRlZFNwZWNTdG9yZS5qYXZh) | `63.76% <66.66%> (+0.27%)` | :arrow_up: |
| [...che/gobblin/runtime/spec\_store/MysqlSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19zdG9yZS9NeXNxbFNwZWNTdG9yZS5qYXZh) | `68.22% <71.42%> (+0.12%)` | :arrow_up: |
| [...apache/gobblin/runtime/spec\_store/FSSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19zdG9yZS9GU1NwZWNTdG9yZS5qYXZh) | `51.75% <100.00%> (+3.64%)` | :arrow_up: |
| [.../modules/scheduler/GobblinServiceJobScheduler.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zY2hlZHVsZXIvR29iYmxpblNlcnZpY2VKb2JTY2hlZHVsZXIuamF2YQ==) | `64.35% <0.00%> (-1.49%)` | :arrow_down: |
| [...lin/elasticsearch/writer/FutureCallbackHolder.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tZWxhc3RpY3NlYXJjaC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9lbGFzdGljc2VhcmNoL3dyaXRlci9GdXR1cmVDYWxsYmFja0hvbGRlci5qYXZh) | `62.85% <0.00%> (+1.42%)` | :arrow_up: |
| [...he/gobblin/metrics/reporter/ScheduledReporter.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0cmljcy9yZXBvcnRlci9TY2hlZHVsZWRSZXBvcnRlci5qYXZh) | `63.63% <0.00%> (+3.03%)` | :arrow_up: |
| [...t/version/TimestampedDatasetStateStoreVersion.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3ZlcnNpb24vVGltZXN0YW1wZWREYXRhc2V0U3RhdGVTdG9yZVZlcnNpb24uamF2YQ==) | `33.33% <0.00%> (+5.55%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d44c12b...4193416](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter edited a comment on pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#issuecomment-899899812
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3367](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (52133d5) into [master](https://codecov.io/gh/apache/gobblin/commit/d44c12bb9e2e73156826de041fd4932da5b610f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d44c12b) will **decrease** coverage by `3.89%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #3367 +/- ##
============================================
- Coverage 46.53% 42.63% -3.90%
+ Complexity 10148 4576 -5572
============================================
Files 2055 1028 -1027
Lines 79894 40871 -39023
Branches 8911 4548 -4363
============================================
- Hits 37177 17427 -19750
+ Misses 39271 21725 -17546
+ Partials 3446 1719 -1727
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...lin/util/filesystem/FileSystemInstrumentation.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsZXN5c3RlbS9GaWxlU3lzdGVtSW5zdHJ1bWVudGF0aW9uLmphdmE=) | `85.71% <0.00%> (-7.15%)` | :arrow_down: |
| [.../modules/scheduler/GobblinServiceJobScheduler.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zY2hlZHVsZXIvR29iYmxpblNlcnZpY2VKb2JTY2hlZHVsZXIuamF2YQ==) | `64.35% <0.00%> (-1.49%)` | :arrow_down: |
| [...java/org/apache/gobblin/hive/HiveSerDeWrapper.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1oaXZlLXJlZ2lzdHJhdGlvbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9oaXZlL0hpdmVTZXJEZVdyYXBwZXIuamF2YQ==) | | |
| [.../org/apache/gobblin/metrics/RootMetricContext.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0cmljcy9Sb290TWV0cmljQ29udGV4dC5qYXZh) | | |
| [.../extractor/hadoop/OldApiHadoopTextInputSource.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvaGFkb29wL09sZEFwaUhhZG9vcFRleHRJbnB1dFNvdXJjZS5qYXZh) | | |
| [...ache/gobblin/converter/csv/CsvToJsonConverter.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9jc3YvQ3N2VG9Kc29uQ29udmVydGVyLmphdmE=) | | |
| [...ce/extractor/limiter/LimiterConfigurationKeys.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc291cmNlL2V4dHJhY3Rvci9saW1pdGVyL0xpbWl0ZXJDb25maWd1cmF0aW9uS2V5cy5qYXZh) | | |
| [...va/org/apache/gobblin/runtime/util/JobMetrics.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvdXRpbC9Kb2JNZXRyaWNzLmphdmE=) | | |
| [...obblin/runtime/listeners/CompositeJobListener.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbGlzdGVuZXJzL0NvbXBvc2l0ZUpvYkxpc3RlbmVyLmphdmE=) | | |
| [...blin/data/management/copy/RecursivePathFinder.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvUmVjdXJzaXZlUGF0aEZpbmRlci5qYXZh) | | |
| ... and [1014 more](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d44c12b...52133d5](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter edited a comment on pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#issuecomment-899899812
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3367](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (52133d5) into [master](https://codecov.io/gh/apache/gobblin/commit/d44c12bb9e2e73156826de041fd4932da5b610f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d44c12b) will **increase** coverage by `0.19%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #3367 +/- ##
============================================
+ Coverage 46.53% 46.72% +0.19%
+ Complexity 10148 3099 -7049
============================================
Files 2055 647 -1408
Lines 79894 25313 -54581
Branches 8911 3011 -5900
============================================
- Hits 37177 11827 -25350
+ Misses 39271 12224 -27047
+ Partials 3446 1262 -2184
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...lin/util/filesystem/FileSystemInstrumentation.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsZXN5c3RlbS9GaWxlU3lzdGVtSW5zdHJ1bWVudGF0aW9uLmphdmE=) | `85.71% <0.00%> (-7.15%)` | :arrow_down: |
| [.../modules/scheduler/GobblinServiceJobScheduler.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zY2hlZHVsZXIvR29iYmxpblNlcnZpY2VKb2JTY2hlZHVsZXIuamF2YQ==) | `64.35% <0.00%> (-1.49%)` | :arrow_down: |
| [...ache/gobblin/metrics/kafka/KafkaEventReporter.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL21ldHJpY3Mva2Fma2EvS2Fma2FFdmVudFJlcG9ydGVyLmphdmE=) | | |
| [.../org/apache/gobblin/metrics/RootMetricContext.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0cmljcy9Sb290TWV0cmljQ29udGV4dC5qYXZh) | | |
| [.../extractor/hadoop/OldApiHadoopTextInputSource.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvaGFkb29wL09sZEFwaUhhZG9vcFRleHRJbnB1dFNvdXJjZS5qYXZh) | | |
| [...ce/extractor/limiter/LimiterConfigurationKeys.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc291cmNlL2V4dHJhY3Rvci9saW1pdGVyL0xpbWl0ZXJDb25maWd1cmF0aW9uS2V5cy5qYXZh) | | |
| [...va/org/apache/gobblin/runtime/util/JobMetrics.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvdXRpbC9Kb2JNZXRyaWNzLmphdmE=) | | |
| [...obblin/runtime/listeners/CompositeJobListener.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbGlzdGVuZXJzL0NvbXBvc2l0ZUpvYkxpc3RlbmVyLmphdmE=) | | |
| [...blin/data/management/copy/RecursivePathFinder.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvUmVjdXJzaXZlUGF0aEZpbmRlci5qYXZh) | | |
| [...e/gobblin/metrics/example/ReporterExampleBase.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0cmljcy9leGFtcGxlL1JlcG9ydGVyRXhhbXBsZUJhc2UuamF2YQ==) | | |
| ... and [1394 more](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d44c12b...52133d5](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] aplex merged pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
aplex merged pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] phet commented on a change in pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
phet commented on a change in pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#discussion_r690792043
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/InstrumentedSpecStore.java
##########
@@ -164,13 +164,26 @@ public Spec updateSpec(Spec spec) throws IOException, SpecNotFoundException {
}
}
+ @Override
+ public int getSize() throws IOException {
+ if (!instrumentationEnabled) {
+ return getSizeImpl();
+ } else {
+ long startTimeMillis = System.currentTimeMillis();
+ int size = getSizeImpl();
+ Instrumented.updateTimer(this.getAllTimer, System.currentTimeMillis() - startTimeMillis, TimeUnit.MILLISECONDS);
Review comment:
looking at something else in this same file, I noticed this (post-merge)...
wouldn't we want a separate `Timer`, to avoid mixing durations from two different operations? I can include that change in what I'm working on now, if you agree @arjun4084346
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter edited a comment on pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#issuecomment-899899812
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3367](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4193416) into [master](https://codecov.io/gh/apache/gobblin/commit/d44c12bb9e2e73156826de041fd4932da5b610f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d44c12b) will **decrease** coverage by `3.48%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #3367 +/- ##
============================================
- Coverage 46.53% 43.04% -3.49%
+ Complexity 10148 1944 -8204
============================================
Files 2055 394 -1661
Lines 79894 16899 -62995
Branches 8911 2082 -6829
============================================
- Hits 37177 7275 -29902
+ Misses 39271 8821 -30450
+ Partials 3446 803 -2643
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...bblin/hive/metastore/HiveMetaStoreEventHelper.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1oaXZlLXJlZ2lzdHJhdGlvbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9oaXZlL21ldGFzdG9yZS9IaXZlTWV0YVN0b3JlRXZlbnRIZWxwZXIuamF2YQ==) | | |
| [...me/spec\_executorInstance/InMemorySpecProducer.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19leGVjdXRvckluc3RhbmNlL0luTWVtb3J5U3BlY1Byb2R1Y2VyLmphdmE=) | | |
| [...ache/gobblin/runtime/job\_spec/ResolvedJobSpec.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvam9iX3NwZWMvUmVzb2x2ZWRKb2JTcGVjLmphdmE=) | | |
| [...modules/flowgraph/datanodes/fs/GridFSDataNode.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9mbG93Z3JhcGgvZGF0YW5vZGVzL2ZzL0dyaWRGU0RhdGFOb2RlLmphdmE=) | | |
| [...retention/CleanableHivePartitionDatasetFinder.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tY29tcGxpYW5jZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9jb21wbGlhbmNlL3JldGVudGlvbi9DbGVhbmFibGVIaXZlUGFydGl0aW9uRGF0YXNldEZpbmRlci5qYXZh) | | |
| [...blin/source/extractor/schema/ColumnAttributes.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3Ivc2NoZW1hL0NvbHVtbkF0dHJpYnV0ZXMuamF2YQ==) | | |
| [...in/converter/objectstore/ObjectStoreConverter.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9vYmplY3RzdG9yZS9PYmplY3RTdG9yZUNvbnZlcnRlci5qYXZh) | | |
| [...source/extractor/extract/kafka/KafkaPartition.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYVBhcnRpdGlvbi5qYXZh) | | |
| [...obblin/compaction/source/CompactionFailedTask.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25GYWlsZWRUYXNrLmphdmE=) | | |
| [...ctor/extract/kafka/KafkaExtractorStatsTracker.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYUV4dHJhY3RvclN0YXRzVHJhY2tlci5qYXZh) | | |
| ... and [1645 more](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d44c12b...4193416](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter edited a comment on pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#issuecomment-899899812
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3367](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4193416) into [master](https://codecov.io/gh/apache/gobblin/commit/d44c12bb9e2e73156826de041fd4932da5b610f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d44c12b) will **increase** coverage by `1.80%`.
> The diff coverage is `64.28%`.
[](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #3367 +/- ##
============================================
+ Coverage 46.53% 48.33% +1.80%
+ Complexity 10148 7522 -2626
============================================
Files 2055 1421 -634
Lines 79894 55949 -23945
Branches 8911 6447 -2464
============================================
- Hits 37177 27043 -10134
+ Misses 39271 26374 -12897
+ Partials 3446 2532 -914
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/gobblin/runtime/api/SpecCatalog.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL1NwZWNDYXRhbG9nLmphdmE=) | `62.71% <0.00%> (ø)` | |
| [.../gobblin/runtime/spec\_catalog/TopologyCatalog.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19jYXRhbG9nL1RvcG9sb2d5Q2F0YWxvZy5qYXZh) | `54.08% <0.00%> (-1.71%)` | :arrow_down: |
| [...ache/gobblin/runtime/spec\_catalog/FlowCatalog.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19jYXRhbG9nL0Zsb3dDYXRhbG9nLmphdmE=) | `49.65% <33.33%> (-0.35%)` | :arrow_down: |
| [...che/gobblin/runtime/api/InstrumentedSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0luc3RydW1lbnRlZFNwZWNTdG9yZS5qYXZh) | `63.76% <66.66%> (+0.27%)` | :arrow_up: |
| [...che/gobblin/runtime/spec\_store/MysqlSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19zdG9yZS9NeXNxbFNwZWNTdG9yZS5qYXZh) | `68.22% <71.42%> (+0.12%)` | :arrow_up: |
| [...apache/gobblin/runtime/spec\_store/FSSpecStore.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvc3BlY19zdG9yZS9GU1NwZWNTdG9yZS5qYXZh) | `51.75% <100.00%> (+3.64%)` | :arrow_up: |
| [...iter/stressTest/RateComputingLimiterContainer.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2UvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2UtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvbGltaXRlci9zdHJlc3NUZXN0L1JhdGVDb21wdXRpbmdMaW1pdGVyQ29udGFpbmVyLmphdmE=) | | |
| [...RecordWithMetadataSchemaRegistrationConverter.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tbWV0YWRhdGEvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL1JlY29yZFdpdGhNZXRhZGF0YVNjaGVtYVJlZ2lzdHJhdGlvbkNvbnZlcnRlci5qYXZh) | | |
| [...ache/gobblin/metrics/kafka/KafkaEventReporter.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL21ldHJpY3Mva2Fma2EvS2Fma2FFdmVudFJlcG9ydGVyLmphdmE=) | | |
| [...va/gobblin/source/extractor/WatermarkInterval.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvV2F0ZXJtYXJrSW50ZXJ2YWwuamF2YQ==) | | |
| ... and [632 more](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d44c12b...4193416](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#discussion_r690836561
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/InstrumentedSpecStore.java
##########
@@ -164,13 +164,26 @@ public Spec updateSpec(Spec spec) throws IOException, SpecNotFoundException {
}
}
+ @Override
+ public int getSize() throws IOException {
+ if (!instrumentationEnabled) {
+ return getSizeImpl();
+ } else {
+ long startTimeMillis = System.currentTimeMillis();
+ int size = getSizeImpl();
+ Instrumented.updateTimer(this.getAllTimer, System.currentTimeMillis() - startTimeMillis, TimeUnit.MILLISECONDS);
Review comment:
Ah. will fix in the next PR. thank you for catching
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] arjun4084346 commented on a change in pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#discussion_r690836561
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/InstrumentedSpecStore.java
##########
@@ -164,13 +164,26 @@ public Spec updateSpec(Spec spec) throws IOException, SpecNotFoundException {
}
}
+ @Override
+ public int getSize() throws IOException {
+ if (!instrumentationEnabled) {
+ return getSizeImpl();
+ } else {
+ long startTimeMillis = System.currentTimeMillis();
+ int size = getSizeImpl();
+ Instrumented.updateTimer(this.getAllTimer, System.currentTimeMillis() - startTimeMillis, TimeUnit.MILLISECONDS);
Review comment:
Ah. will fix in the next PR. thank you for catching
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter commented on pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#issuecomment-899899812
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3367](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (52133d5) into [master](https://codecov.io/gh/apache/gobblin/commit/d44c12bb9e2e73156826de041fd4932da5b610f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d44c12b) will **decrease** coverage by `3.48%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #3367 +/- ##
============================================
- Coverage 46.53% 43.04% -3.49%
+ Complexity 10148 1944 -8204
============================================
Files 2055 394 -1661
Lines 79894 16899 -62995
Branches 8911 2082 -6829
============================================
- Hits 37177 7274 -29903
+ Misses 39271 8821 -30450
+ Partials 3446 804 -2642
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...lin/util/filesystem/FileSystemInstrumentation.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsZXN5c3RlbS9GaWxlU3lzdGVtSW5zdHJ1bWVudGF0aW9uLmphdmE=) | `85.71% <0.00%> (-7.15%)` | :arrow_down: |
| [.../compaction/mapreduce/MRCompactionTaskFactory.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vbWFwcmVkdWNlL01SQ29tcGFjdGlvblRhc2tGYWN0b3J5LmphdmE=) | | |
| [...g/apache/gobblin/restli/throttling/NoopPolicy.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2UvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2Utc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3Jlc3RsaS90aHJvdHRsaW5nL05vb3BQb2xpY3kuamF2YQ==) | | |
| [...bblin/recordaccess/FieldDoesNotExistException.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vcmVjb3JkYWNjZXNzL0ZpZWxkRG9lc05vdEV4aXN0RXhjZXB0aW9uLmphdmE=) | | |
| [...apache/gobblin/compliance/HivePartitionFinder.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tY29tcGxpYW5jZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9jb21wbGlhbmNlL0hpdmVQYXJ0aXRpb25GaW5kZXIuamF2YQ==) | | |
| [...ersion/finder/GlobModTimeDatasetVersionFinder.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L3ZlcnNpb24vZmluZGVyL0dsb2JNb2RUaW1lRGF0YXNldFZlcnNpb25GaW5kZXIuamF2YQ==) | | |
| [...va/org/apache/gobblin/metadata/types/Metadata.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tbWV0YWRhdGEvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0YWRhdGEvdHlwZXMvTWV0YWRhdGEuamF2YQ==) | | |
| [...modules/flowgraph/datanodes/fs/GridFSDataNode.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9mbG93Z3JhcGgvZGF0YW5vZGVzL2ZzL0dyaWRGU0RhdGFOb2RlLmphdmE=) | | |
| [...che/gobblin/data/management/copy/CopyableFile.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvQ29weWFibGVGaWxlLmphdmE=) | | |
| [...ogle/webmaster/GoogleWebmasterDataFetcherImpl.java](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvb2dsZS1pbmdlc3Rpb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vaW5nZXN0aW9uL2dvb2dsZS93ZWJtYXN0ZXIvR29vZ2xlV2VibWFzdGVyRGF0YUZldGNoZXJJbXBsLmphdmE=) | | |
| ... and [1646 more](https://codecov.io/gh/apache/gobblin/pull/3367/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d44c12b...52133d5](https://codecov.io/gh/apache/gobblin/pull/3367?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] phet commented on a change in pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
phet commented on a change in pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#discussion_r690607379
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/SpecCatalog.java
##########
@@ -110,7 +112,7 @@ public StandardMetrics(final SpecCatalog specCatalog, Optional<Config> sysConfig
this.totalUpdatedSpecs = new AtomicLong(0);
this.numActiveSpecs = metricsContext.newContextAwareGauge(NUM_ACTIVE_SPECS_NAME, ()->{
long startTime = System.currentTimeMillis();
- int size = specCatalog.getSpecs().size();
+ int size = specCatalog.getSize();
Review comment:
incredible! when we spoke, I had imagined the copious deserialization to originate in the need to peek inside the `Spec` (e.g. to selectively count by filtering in java-space).
I'm absolutely floored that sampling showed this (teeny-tiny) invocation to occupy 42% of CPU time (on the leader) while accounting for 77.5% of memory allocations
excellent work, arjun!
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/spec_catalog/TopologyCatalog.java
##########
@@ -232,6 +232,15 @@ public void switchMetricContext(MetricContext context) {
}
}
+ @Override
+ public int getSize() {
+ try {
+ return specStore.getSize();
+ } catch (IOException e) {
+ throw new RuntimeException("Cannot retrieve number of specs from Spec store", e);
+ }
+ }
Review comment:
[thought for later] literal duplication (in each containing class): perhaps explore a `CheckedExceptionWrappingSpecStore`, basically a Decorator for centralizing this? doing so lends similar benefit as the `InstrumentedSpecStore` by encapsulating timing in one place (although based on composition rather than impl inheritance, as ISS is).
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/SpecCatalog.java
##########
@@ -51,6 +51,8 @@
* */
Review comment:
prophetic comment, if there ever was one...
now that you've reworked the metrics gauge, the only remaining use seems to be in managing listeners. AFAICT (in my budding understanding) that appears reasonable, but might be worth you confirming.
(presuming this optimization bears the copious fruit we hope for) shall we return to update this comment, to scare off casually-considered use? although perhaps a slight abusage of `@Deprecated` we could shout that caution as a build-time warning. what's your take?
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/InstrumentedSpecStore.java
##########
@@ -164,13 +164,26 @@ public Spec updateSpec(Spec spec) throws IOException, SpecNotFoundException {
}
}
+ @Override
+ public int getSize() throws IOException {
+ if (!instrumentationEnabled) {
+ return getSizeImpl();
+ } else {
+ long startTimeMillis = System.currentTimeMillis();
+ int size = getSizeImpl();
+ Instrumented.updateTimer(this.getAllTimer, System.currentTimeMillis() - startTimeMillis, TimeUnit.MILLISECONDS);
Review comment:
looking at something else in this same file, I noticed this (post-merge)...
wouldn't we want a separate `Timer`, to avoid mixing durations from two different operations? I can include that change in what I'm working on now, if you agree @arjun4084346
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/InstrumentedSpecStore.java
##########
@@ -164,13 +164,26 @@ public Spec updateSpec(Spec spec) throws IOException, SpecNotFoundException {
}
}
+ @Override
+ public int getSize() throws IOException {
+ if (!instrumentationEnabled) {
+ return getSizeImpl();
+ } else {
+ long startTimeMillis = System.currentTimeMillis();
+ int size = getSizeImpl();
+ Instrumented.updateTimer(this.getAllTimer, System.currentTimeMillis() - startTimeMillis, TimeUnit.MILLISECONDS);
Review comment:
cool, no worries. I added it along with my recent PR already in progress - https://github.com/apache/gobblin/pull/3369
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] aplex merged pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
aplex merged pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a change in pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
umustafi commented on a change in pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#discussion_r690554732
##########
File path: gobblin-runtime/src/test/java/org/apache/gobblin/runtime/spec_catalog/FlowCatalogTest.java
##########
@@ -191,6 +192,7 @@ public void deleteFlowSpec() throws SpecNotFoundException {
logger.info("[After Delete] Spec " + i++ + ": " + gson.toJson(flowSpec));
}
Assert.assertTrue(specs.size() == 0, "Spec store should be empty after deletion");
+ Assert.assertEquals(flowCatalog.getSize(), 0, "Spec store should be empty after deletion");
Review comment:
in these places we can change them to assertEquals as well
##########
File path: gobblin-runtime/src/test/java/org/apache/gobblin/runtime/spec_catalog/FlowCatalogTest.java
##########
@@ -161,6 +161,7 @@ public void createFlowSpec() {
logger.info("[After Create] Spec " + i++ + ": " + gson.toJson(flowSpec));
}
Assert.assertTrue(specs.size() == 1, "Spec store should contain 1 Spec after addition");
Review comment:
nit: for code quality should we mirror these assertions to use either assertEquals or assertTrue? I prefer the former (the way you did it)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] phet commented on a change in pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
phet commented on a change in pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#discussion_r690607379
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/SpecCatalog.java
##########
@@ -110,7 +112,7 @@ public StandardMetrics(final SpecCatalog specCatalog, Optional<Config> sysConfig
this.totalUpdatedSpecs = new AtomicLong(0);
this.numActiveSpecs = metricsContext.newContextAwareGauge(NUM_ACTIVE_SPECS_NAME, ()->{
long startTime = System.currentTimeMillis();
- int size = specCatalog.getSpecs().size();
+ int size = specCatalog.getSize();
Review comment:
incredible! when we spoke, I had imagined the copious deserialization to originate in the need to peek inside the `Spec` (e.g. to selectively count by filtering in java-space).
I'm absolutely floored that sampling showed this (teeny-tiny) invocation to occupy 42% of CPU time (on the leader) while accounting for 77.5% of memory allocations
excellent work, arjun!
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/spec_catalog/TopologyCatalog.java
##########
@@ -232,6 +232,15 @@ public void switchMetricContext(MetricContext context) {
}
}
+ @Override
+ public int getSize() {
+ try {
+ return specStore.getSize();
+ } catch (IOException e) {
+ throw new RuntimeException("Cannot retrieve number of specs from Spec store", e);
+ }
+ }
Review comment:
[thought for later] literal duplication (in each containing class): perhaps explore a `CheckedExceptionWrappingSpecStore`, basically a Decorator for centralizing this? doing so lends similar benefit as the `InstrumentedSpecStore` by encapsulating timing in one place (although based on composition rather than impl inheritance, as ISS is).
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/SpecCatalog.java
##########
@@ -51,6 +51,8 @@
* */
Review comment:
prophetic comment, if there ever was one...
now that you've reworked the metrics gauge, the only remaining use seems to be in managing listeners. AFAICT (in my budding understanding) that appears reasonable, but might be worth you confirming.
(presuming this optimization bears the copious fruit we hope for) shall we return to update this comment, to scare off casually-considered use? although perhaps a slight abusage of `@Deprecated` we could shout that caution as a build-time warning. what's your take?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a change in pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
umustafi commented on a change in pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#discussion_r690554732
##########
File path: gobblin-runtime/src/test/java/org/apache/gobblin/runtime/spec_catalog/FlowCatalogTest.java
##########
@@ -191,6 +192,7 @@ public void deleteFlowSpec() throws SpecNotFoundException {
logger.info("[After Delete] Spec " + i++ + ": " + gson.toJson(flowSpec));
}
Assert.assertTrue(specs.size() == 0, "Spec store should be empty after deletion");
+ Assert.assertEquals(flowCatalog.getSize(), 0, "Spec store should be empty after deletion");
Review comment:
in these places we can change them to assertEquals as well
##########
File path: gobblin-runtime/src/test/java/org/apache/gobblin/runtime/spec_catalog/FlowCatalogTest.java
##########
@@ -161,6 +161,7 @@ public void createFlowSpec() {
logger.info("[After Create] Spec " + i++ + ": " + gson.toJson(flowSpec));
}
Assert.assertTrue(specs.size() == 1, "Spec store should contain 1 Spec after addition");
Review comment:
nit: for code quality should we mirror these assertions to use either assertEquals or assertTrue? I prefer the former (the way you did it)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] phet commented on a change in pull request #3367: [GOBBLIN-1518] create getSize api for spec store which is faster than doing getAll a…
Posted by GitBox <gi...@apache.org>.
phet commented on a change in pull request #3367:
URL: https://github.com/apache/gobblin/pull/3367#discussion_r690867668
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/InstrumentedSpecStore.java
##########
@@ -164,13 +164,26 @@ public Spec updateSpec(Spec spec) throws IOException, SpecNotFoundException {
}
}
+ @Override
+ public int getSize() throws IOException {
+ if (!instrumentationEnabled) {
+ return getSizeImpl();
+ } else {
+ long startTimeMillis = System.currentTimeMillis();
+ int size = getSizeImpl();
+ Instrumented.updateTimer(this.getAllTimer, System.currentTimeMillis() - startTimeMillis, TimeUnit.MILLISECONDS);
Review comment:
cool, no worries. I added it along with my recent PR already in progress - https://github.com/apache/gobblin/pull/3369
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org