SAMZA-2397: Updating gauge-val function on newGauge on same metric name #1223

rmatharu-zz · 2019-11-22T04:32:50Z

Updating gauge-val function on newGauge on same metric name.

API Change: MetricRegistryMap. newGauge will now overwrite any existing gauge, if one exists for the same group and gauge.name.

Symptom: RocksDB-specific metric value like estimate-table-readers-mem, cur-size-active-mem-table, etc, are not being emitted after Samza 1.1; only a 0 value is emitted instead.

Cause: Samza 1.1 uses an optimization where a RocksDBStore is created first in BulkLoad mode for optimizing bootstrap write, after bootstrapping is complete the store is closed (to trigger compaction), and a new store is created in ReadWrite mode. The rocksdb metrics are calculated using a function which checks if the store is open (see #588 by @xinyuiscool , SAMZA-1778), otherwise it emits 0.
See RocksdbKeyValueStore#95.
However, because the metric-gauge-object is not updated after the store is recreated, the gauge keeps checking the first (closed) store and emits 0.

Fix: Added logic that ensures that the second store's metric overrides the previous gauge for the same name, and logs a message.

Tests: Added unit test, test with a samza job that emits non-zero RocksDB metrics.

prateekm · 2019-11-22T18:22:52Z

@bkonold can you review?

Updating method in MetricsRegistryMap

bkonold · 2019-11-26T10:23:31Z

samza-api/src/main/java/org/apache/samza/metrics/MetricsRegistry.java

+   * @param overrideExistingGauge Overwrite any existing gauges present for the same group and gauge
+   * @return Gauge was registered
+   */
+  <T> Gauge<T> newGauge(String group, Gauge<T> value, Boolean overrideExistingGauge);


Are we defining a new method here with the additional parameter to avoid a large code change in adding a boolean to the definition above?

Wondering whether the two methods will coexist long term or if we see a way to unify eventually.

Yes, making overriding to be the default behavior requires testing to ensure that it doesnt break existing metrics; I can work on that separately.
Although I tried with a test-job and everything looked ok.

I'd prefer to not add a new variant here and update the existing behavior instead. It seems to fall into "fixing undefined behavior" territory, in that I don't think a reasonable user expectation would be to retain the old metric. We can fix this here and call it out in the "backwards incompatible / API changes" section of the PR description.

@cameronlee314 What do you think?

If leaving them separate is what is needed to ensure the P2 timeline and we plan for the two living along side one another to be temporary, maybe we can open a ticket and indicate in a comment when / under what conditions clean up can happen?

Done, added details in https://issues.apache.org/jira/browse/SAMZA-2417

I'd prefer to not add a new variant here and update the existing behavior instead. It seems to fall into "fixing undefined behavior" territory, in that I don't think a reasonable user expectation would be to retain the old metric. We can fix this here and call it out in the "backwards incompatible / API changes" section of the PR description.

@cameronlee314 What do you think?

I was debating doing that, but I'm not sure if it'd break some existing metrics-subsystem which relies on us retaining the first object rather than the last?

@prateekm I rollbacked all most of the changes; we now just retain the newest metric.
can you approve it if it looks ok, so we can merge.

bkonold · 2019-12-10T20:38:51Z

samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java

    });
  }

+  public <T> Gauge<T> newGauge(String name, final ValueFunction<T> valueFunc, Boolean overwrite) {


There seems to be a mixing of terminology here vs MetricsRegistry (overwrite vs overrideExistingGauge). All we're doing is passing the arg here on to the registry; is the difference in naming intentional?

done, made naming consistent.

samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala

samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala

… coordinator (apache#1173)

…1232)

…e#1231) doc and blog related update on master branch for 1.3.0 release

…SamzaSqlRelRecord for all the nested records.

…ped during a rebalance (apache#1213) Stream processor should ensure previous container is stopped during a rebalance

…ache#1234)

…rams to zopkio (apache#1235)

…ing env variables in ShellCommandConfig Design: https://cwiki.apache.org/confluence/display/SAMZA/SEP-23%3A+Simplify+Job+Runner Changes: 1. Add job.config.loader.factory to specify ConfigLoaderFactory. 2. Add job.config.loader.properties prefix to specify properties needed by ConfigLoader to fetch config. 3. Add ENV_SUBMISSION_CONFIG enviroment variable to pass job submission configs to Yarn. API Changes: N/A. This is part of a series PRs, detailed information will be provided in the last/main PR. Upgrade Instructions: N/A. This is part of a series PRs, detailed information will be provided in the last/main PR. Usage Instructions: N/A. This is part of a series PRs, detailed information will be provided in the last/main PR. Tests: N/A.

…ache#1240) Populate container exception from the listener only if it is null

…lt PropertiesConfigLoader as well as PropertiesConfigLoaderFactory implementations. Add ConfigLoader, ConfigLoaderFactory interface and default PropertiesConfigLoader as well as PropertiesConfigLoaderFactory implementations, which will be used on AM to fetch configs. It will also be used on LocalApplicationRunner to fetch configs as well. The existing ConfigFactory interface will be deprecated and removed as we do not fetch configs on start up script run-app.sh anymore. Design: https://cwiki.apache.org/confluence/display/SAMZA/SEP-23%3A+Simplify+Job+Runner Changes: 1. Add ConfigLoader interface which will be used in job coordinator to retrieve config from. 2. Add ConfigLoaderFactory interface which will be used to create an instance of ConfigLoader. 3. Add PropertiesConfigLoader which is a default implementation of ConfigLoader that reads a local properties file given the file path. 4. Add PropertiesConfigLoaderFactory which is a default implementation of ConfigLoaderFactory that reads properties file path from start up config and creates a PropertiesConfigLoader with the provided path. API Changes: N/A. This is part of a series PRs, detailed information will be provided in the last/main PR. Upgrade Instructions: N/A. This is part of a series PRs, detailed information will be provided in the last/main PR. Usage Instructions: N/A. This is part of a series PRs, detailed information will be provided in the last/main PR. Tests: Add TestPropertiesConfigLoader to unit test PropertiesConfigLoader.

…ctory

* Refactor LocalApplicationRunner Issues: LocalApplicationRunner has multiple constructors and follows different initialization logic. Changes: 1. Refactor all constructors to end up with the same private constructor. 2. Align "static final" and "final static" to "static final" 3. Group class variales based on accessibility 4. Minor fixs on inconsistent styling, return etc. Tests: Unit Tests * Revert LocalApplicationRunner(SamzaApplication app, Config config, MetadataStoreFactory metadataStoreFactory) from package private back to public

…che#1245) Issues: 1. org.apache.samza.sql.util.ConfigUtil is not used 2. it is the same name as org.apache.samza.util.ConfigUtil 3. its usage should be deprecated in favor of Config#subset(String prefix, boolean stripPrefix) Changes: delete org.apache.samza.sql.util.ConfigUtil Tests: build

Issues: samza-autoscaling module is not used. Changes: remove the unused module. API Changes: None Upgrade Instructions: None Usage Instructions: None Tests: build

…ache#1251) * Fix the checkpoint and changelog topic creation configurations. * Address review comments. * Address review comments.

…on. (apache#1254)

…pache#1246)

…#1247) 1. Depending on the existence of job.config.loader.factory, RemoteApplicationRunner will alternatively only submit the job, without planning. 2. Late initialize RemoteJobPlanner only when needed to avoid executing user code in submitting only mode.

…from loader. (apache#1248) 1. Based on the existence of ENV_SUBMISSION_CONFIG, ClusterBasedJobCoordinator will alternatively deserilize ENV_SUBMISSION_CONFIG and load full job config from config loader factory. 2. Execute planning, create diagnostics stream and persist full job config back to coordinator stream when loading full job config from config loader.

prateekm

Thanks for the update. Fix it and ship it.

Let's update the PR description with the "API changes" etc. sections (from SEP-25) and call this out as an API change.

prateekm · 2020-01-21T18:30:18Z

samza-core/src/main/scala/org/apache/samza/metrics/MetricsRegistryMap.scala

-    debug("Adding new gauge %s %s %s." format (group, gauge.getName, gauge))
-    putAndGetGroup(group).putIfAbsent(gauge.getName, gauge)
+    if (putAndGetGroup(group).containsKey(gauge.getName)) {
+      info("Updating existing gauge %s %s %s" format (group, gauge.getName, gauge))


Should be debug.

prateekm · 2020-01-21T18:30:23Z

samza-core/src/main/scala/org/apache/samza/metrics/MetricsRegistryMap.scala


 package org.apache.samza.metrics

+import java.lang


rmatharu added 3 commits November 21, 2019 20:30

SAMZA-2397: Updating gauge-val function on newGauge on same metric name

8cad71d

Adding unit test for metrics

f64d6cf

Adding separate method on MetricsHelper to get new gauge with overwrite

7498855

Adding separate method on MetricsHelper to get new gauge with overwrite

245eefd

Updating method in MetricsRegistryMap

bkonold reviewed Nov 26, 2019

View reviewed changes

Empty commit to trigger build

4764ade

bkonold reviewed Dec 10, 2019

View reviewed changes

Addressing review comments

959b3d8

bkonold reviewed Dec 12, 2019

View reviewed changes

samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala Outdated Show resolved Hide resolved

bkonold reviewed Dec 12, 2019

View reviewed changes

samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala Outdated Show resolved Hide resolved

rmatharu and others added 19 commits December 12, 2019 16:02

Minor naming changes

16256fb

Trigger build

7e558bf

SAMZA-2333: [AM isolation] Use cytodynamics classloader to launch job…

dfb4cee

… coordinator (apache#1173)

update release instruction to include integration tests (apache#1224)

d2e0375

SAMZA-2403: fix standalone integration failure (apache#1225)

069a8c0

SAMZA-2388: [Scala cleanup] Convert Util to Java (apache#1221)

20eb345

update gradle properties to mark 1.4.0 as the next version (apache#1230)

5e9244a

[DOCS] Update kafka consumer command to use bootstrap-server (apache#…

6d206d5

…1232)

doc and blog related update on master branch for 1.3.0 release (apach…

c8417b7

…e#1231) doc and blog related update on master branch for 1.3.0 release

Fix AvroRelConverter to only consider cached schema while populating …

c55e558

…SamzaSqlRelRecord for all the nested records.

SAMZA-2305: Stream processor should ensure previous container is stop…

d181ed8

…ped during a rebalance (apache#1213) Stream processor should ensure previous container is stopped during a rebalance

Adding null-check before incrementing metrics for bytesSerialized (ap…

1854e3a

…ache#1234)

SAMZA-2418: Integration tests are failing and do not properly pass pa…

b53cb14

…rams to zopkio (apache#1235)

Update javadoc

78b6150

Add getConfigLoaderFactory() for easy access to config loader factory.

fdee896

SAMZA-2423: Heartbeat failure causes incorrect container shutdown (ap…

3e72812

…ache#1240) Populate container exception from the listener only if it is null

SAMZA-2425: Add support for sub-query in joins

f1e0a35

Revert indentions change

77499b7

Ke Wu and others added 24 commits January 20, 2020 19:20

Fix typo and switch to Optional for getConfigLoaderFactory

bfb4d43

Add license info

6d841d2

Update javadoc

30875ac

Remove unused import

e24df2b

Update JavaDocs

730dbcb

Add javadoc for PropertiesConfigLoader

bd0c537

Update javadoc

677f7f5

Move CONFIG_LOADER_PROPERTIES_PREFIX from JobConfig to ConfigLoaderFa…

c491a13

…ctory

Add "strict" parameter to all the log4j.xml files (apache#1244)

6cf114a

Clean up unused org.apache.samza.autoscaling module (apache#1250)

c2c73dd

Issues: samza-autoscaling module is not used. Changes: remove the unused module. API Changes: None Upgrade Instructions: None Usage Instructions: None Tests: build

Fix the coordinator stream creation workflow.

afcb11c

SAMZA-2431: Fix the checkpoint and changelog topic auto-creation. (ap…

a35de71

…ache#1251) * Fix the checkpoint and changelog topic creation configurations. * Address review comments. * Address review comments.

Disable Flaky Tests

405d8a6

SAMZA-2421: Add SystemProducer for Azure Blob Storage (apache#1239)

c867cd3

Fix the RocksDB TTL type conversion in change log properties generati…

09ac78c

…on. (apache#1254)

SAMZA-2409: Update YarnJob to construct job submission env variable (a…

c780c1a

…pache#1246)

Removing additional method

dec12ff

Merge branch 'master' into test-rocksdb-metric-fix

dfba824

Undoing MetricGroup change

e9bd445

prateekm approved these changes Jan 21, 2020

View reviewed changes

Minor

1f50662

rmatharu-zz merged commit d20b9b3 into apache:master Jan 21, 2020

prateekm mentioned this pull request Mar 6, 2023

Side Inputs + Blob Store Backups - Part 1 - Fix duplicate uploads and broken metrics #1654

Merged

SAMZA-2397: Updating gauge-val function on newGauge on same metric name #1223

SAMZA-2397: Updating gauge-val function on newGauge on same metric name #1223

Uh oh!

Conversation

rmatharu-zz commented Nov 22, 2019 • edited by prateekm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prateekm commented Nov 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmatharu-zz Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

prateekm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

rmatharu-zz commented Nov 22, 2019 •

edited by prateekm

Loading

rmatharu-zz Dec 2, 2019 •

edited

Loading