[HUDI-6313] Add metrics counters for compaction requested/completed events. #8759

amrishlal · 2023-05-18T21:12:17Z

Change Logs

Add metrics counters for compaction start/stop events so that we can keep track of how many compactions were requested, how many finished, and how many produced error (interfered as number of starts - number of finished).

Impact

No user api for performance impact expected.

Risk level (write none, low medium or high below)

Low

Documentation Update

None

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

SteNicholas · 2023-05-19T04:08:34Z

@amrishlal, I don't think it's necessary to introduce the metrics in this pull request. The essence of demand in description is that monitor the different state of compaction action. Therefore, we should introduce different state metric of compaction action in state change phase.

nsivabalan

1 minor comment

nsivabalan · 2023-05-22T23:28:41Z

...t-common/src/main/java/org/apache/hudi/table/action/compact/RunCompactionActionExecutor.java

  }

  @Override
  public HoodieWriteMetadata<HoodieData<WriteStatus>> execute() {
+    LOG.info("Compaction start.");


do we need this. or you added for debugging purpose?

@nsivabalan I added this based on our last discussion. It should help to get an idea of individual start / stop compaction events.

nsivabalan · 2023-05-22T23:31:17Z

@SteNicholas : I get your intention. we did take a look at the active timeline methods where we do the transition. As of now, HoodieActive timeline is lightweight and does not have much dependency w/ other hudi entities as much as possible. we don't want to add dep to HoodieMetrics within Active timeline. Especially given we wanted to add this only for compaction. If demand arises that we want to emit this metric for all operations, we can think about it.
but for now, may be we can keep it one of and leave it as is. let us know what do you think.

amrishlal · 2023-05-23T17:41:14Z

@SteNicholas Just pinging to see if you are ok with this PR based on @nsivabalan comments above?

SteNicholas · 2023-05-24T03:23:46Z

@amrishlal, @nsivabalan, could the compaction.start and compaction.stop update to compaction.requested compaction.completed? compaction.stop make certain confusing to me.

amrishlal · 2023-05-24T18:22:43Z

@amrishlal, @nsivabalan, could the compaction.start and compaction.stop update to compaction.requested compaction.completed? compaction.stop make certain confusing to me.

@SteNicholas Changed names to compaction.requested compaction.completed.

SteNicholas · 2023-05-24T20:31:47Z

@amrishlal, could you add the test cause to verify whether the metric value is correct? Meanwhile, please change the title of this pull request to Add metrics counters for compaction requested/completed events.

amrishlal · 2023-05-24T23:51:54Z

@amrishlal, could you add the test cause to verify whether the metric value is correct? Meanwhile, please change the title of this pull request to Add metrics counters for compaction requested/completed events.

@SteNicholas Can you point me to any similar test cases (where operational metrics are verified) that I can follow?

SteNicholas · 2023-05-25T03:37:47Z

@amrishlal, you could follow TestHoodieMetrics to verify the introduced metrics. PTAL.
cc @nsivabalan

… compaction test case.

amrishlal · 2023-05-26T22:09:22Z

@amrishlal, you could follow TestHoodieMetrics to verify the introduced metrics. PTAL. cc @nsivabalan

@SteNicholas I have added checks for compaction.requested and compaction.completed metrics as part of existing tests in TestHoodieCompactor.java

SteNicholas

LGTM. cc @nsivabalan

nsivabalan · 2023-05-30T19:40:38Z

...t-common/src/main/java/org/apache/hudi/table/action/compact/RunCompactionActionExecutor.java

@@ -48,10 +52,14 @@
 public class RunCompactionActionExecutor<T> extends
    BaseActionExecutor<T, HoodieData<HoodieRecord<T>>, HoodieData<HoodieKey>, HoodieData<WriteStatus>, HoodieWriteMetadata<HoodieData<WriteStatus>>> {

+  private static final Logger LOG = LoggerFactory.getLogger(LogCompactionExecutionHelper.class);
+


RunCompactionActionExecutor.class

nsivabalan · 2023-05-30T19:41:04Z

...t-common/src/main/java/org/apache/hudi/table/action/compact/RunCompactionActionExecutor.java

  }

  @Override
  public HoodieWriteMetadata<HoodieData<WriteStatus>> execute() {
+    LOG.info("Compaction requested.");


can we add the instant time for compaction

nsivabalan · 2023-05-30T19:41:08Z

...t-common/src/main/java/org/apache/hudi/table/action/compact/RunCompactionActionExecutor.java

@@ -116,6 +128,8 @@ public HoodieWriteMetadata<HoodieData<WriteStatus>> execute() {
      throw new HoodieCompactionException("Could not compact " + config.getBasePath(), e);
    }

+    LOG.info("Compaction completed.");


can we add the instant time for compaction

nsivabalan · 2023-05-30T20:40:32Z

...udi-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestHoodieCompactor.java

@@ -129,6 +152,10 @@ public void testCompactionEmpty() {
      String compactionInstantTime = HoodieActiveTimeline.createNewInstantTime();
      Option<HoodieCompactionPlan> plan = table.scheduleCompaction(context, compactionInstantTime, Option.empty());
      assertFalse(plan.isPresent(), "If there is nothing to compact, result will be empty");
+
+      // Verify compaction.requested, compaction.completed metrics counts.
+      assertEquals(0, getCompactionMetricCount("counter", "compaction.requested"));


can we re-use the variables here instead of hard coding the metric names ?

I can use const variables, but from what I could see referring to metric names using inline literal strings appears to be the common pattern in the codebase and hence this usage. Let me know what you think.

@amrishlal, you could use const variable REQUESTED_COMPACTION_EXTENSION in HoodieTimeline.

@SteNicholas Using HoodieTimeline.REQUESTED_COMPACTION_SUFFIX and HoodieTimeline.COMPLETED_COMPACTION_SUFFIX as REQUESTED_COMPACTION_EXTENSION was leading to a double dot in the final metric name.

nsivabalan · 2023-05-30T20:40:37Z

...udi-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestHoodieCompactor.java

@@ -180,6 +208,10 @@ public void testWriteStatusContentsAfterCompaction() throws Exception {
      HoodieData<WriteStatus> result = compact(writeClient, compactionInstantTime);

      verifyCompaction(result);
+
+      // Verify compaction.requested, compaction.completed metrics counts.
+      assertEquals(1, getCompactionMetricCount("counter", "compaction.requested"));


responded above.

SteNicholas · 2023-06-02T05:13:11Z

@amrishlal, could you create a JIRA ticket or issue for this metric introduction? Meanwhile, you should update the title of this pull request, otherwise this pull request fails to validate.

amrishlal · 2023-06-02T17:04:09Z

@amrishlal, could you create a JIRA ticket or issue for this metric introduction? Meanwhile, you should update the title of this pull request, otherwise this pull request fails to validate.

Done.

hudi-bot · 2023-06-20T21:04:34Z

CI report:

eab8998 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

Add metrics counters for compaction start/stop events.

953795e

amrishlal marked this pull request as ready for review May 18, 2023 21:33

Fix code formatting.

fbdd1d2

nsivabalan reviewed May 22, 2023

View reviewed changes

Modify metric names.

f926796

amrishlal changed the title ~~Add metrics counters for compaction start/stop events.~~ Add metrics counters for compaction requested/completed events. May 24, 2023

Verify compaction.requested and compaction.completed metric values in…

eb78efa

… compaction test case.

SteNicholas approved these changes May 29, 2023

View reviewed changes

nsivabalan reviewed May 30, 2023

View reviewed changes

code review changes.

c8b0280

const variables for compaction metric names.

155e496

amrishlal changed the title ~~Add metrics counters for compaction requested/completed events.~~ [HUDI-6313] Add metrics counters for compaction requested/completed events. Jun 2, 2023

Merge branch 'apache:master' into add-compaction-metrics

eab8998

nsivabalan approved these changes Jun 20, 2023

View reviewed changes

nsivabalan merged commit 35897ba into apache:master Jun 20, 2023
44 checks passed

amrishlal deleted the add-compaction-metrics branch June 20, 2023 23:02

stream2000 mentioned this pull request Nov 17, 2023

[HUDI-7099] Providing metrics for archive and defining some string constants #10101

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-6313] Add metrics counters for compaction requested/completed events. #8759

[HUDI-6313] Add metrics counters for compaction requested/completed events. #8759

amrishlal commented May 18, 2023

SteNicholas commented May 19, 2023

nsivabalan left a comment

nsivabalan May 22, 2023

amrishlal May 23, 2023

nsivabalan commented May 22, 2023

amrishlal commented May 23, 2023

SteNicholas commented May 24, 2023 •

edited

Loading

amrishlal commented May 24, 2023

SteNicholas commented May 24, 2023 •

edited

Loading

amrishlal commented May 24, 2023

SteNicholas commented May 25, 2023 •

edited

Loading

amrishlal commented May 26, 2023

SteNicholas left a comment

nsivabalan May 30, 2023

amrishlal Jun 1, 2023

nsivabalan May 30, 2023

amrishlal Jun 1, 2023

nsivabalan May 30, 2023

amrishlal Jun 1, 2023

nsivabalan May 30, 2023

amrishlal Jun 1, 2023 •

edited

Loading

SteNicholas Jun 2, 2023

amrishlal Jun 2, 2023

nsivabalan May 30, 2023

amrishlal Jun 1, 2023

SteNicholas commented Jun 2, 2023

amrishlal commented Jun 2, 2023

hudi-bot commented Jun 20, 2023

[HUDI-6313] Add metrics counters for compaction requested/completed events. #8759

[HUDI-6313] Add metrics counters for compaction requested/completed events. #8759

Conversation

amrishlal commented May 18, 2023

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

SteNicholas commented May 19, 2023

nsivabalan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsivabalan commented May 22, 2023

amrishlal commented May 23, 2023

SteNicholas commented May 24, 2023 • edited Loading

amrishlal commented May 24, 2023

SteNicholas commented May 24, 2023 • edited Loading

amrishlal commented May 24, 2023

SteNicholas commented May 25, 2023 • edited Loading

amrishlal commented May 26, 2023

SteNicholas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amrishlal Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteNicholas commented Jun 2, 2023

amrishlal commented Jun 2, 2023

hudi-bot commented Jun 20, 2023

CI report:

SteNicholas commented May 24, 2023 •

edited

Loading

SteNicholas commented May 24, 2023 •

edited

Loading

SteNicholas commented May 25, 2023 •

edited

Loading

amrishlal Jun 1, 2023 •

edited

Loading