[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…

[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