[
https://issues.apache.org/jira/browse/GOBBLIN-1518?focusedWorklogId=638815&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-638815
]
ASF GitHub Bot logged work on GOBBLIN-1518:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 17/Aug/21 18:42
Start Date: 17/Aug/21 18:42
Worklog Time Spent: 10m
Work Description: 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 638815)
Time Spent: 1h 40m (was: 1.5h)
> improve getting size of the spec store
> --------------------------------------
>
> Key: GOBBLIN-1518
> URL: https://issues.apache.org/jira/browse/GOBBLIN-1518
> Project: Apache Gobblin
> Issue Type: Improvement
> Reporter: Arjun Singh Bora
> Priority: Major
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> 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
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
