[ISSUE-378][HugePartition][Part-3] Introduce more metrics about huge partition by zuston · Pull Request #494 · apache/uniffle · GitHub
Skip to content

[ISSUE-378][HugePartition][Part-3] Introduce more metrics about huge partition#494

Merged
zuston merged 6 commits into
apache:masterfrom
zuston:master
Jan 22, 2023
Merged

[ISSUE-378][HugePartition][Part-3] Introduce more metrics about huge partition#494
zuston merged 6 commits into
apache:masterfrom
zuston:master

Conversation

@zuston

@zuston zuston commented Jan 17, 2023

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Introduce more metrics about huge partition
counter

  1. total_require_buffer_failed_for_huge_partition
  2. total_require_buffer_failed_for_regular_partition
  3. total_app_num
  4. total_app_with_huge_partition_num
  5. total_partition_num
  6. total_huge_partition_num

Gauge

  1. huge_partition_num
  2. app_with_huge_partition_num

Why are the changes needed?

Having these metrics, we should observe the concrete influence from huge partition for regular partition and huge partition number in one shuffle-server

Does this PR introduce any user-facing change?

No

How was this patch tested?

  1. UTs

@codecov-commenter

codecov-commenter commented Jan 17, 2023

Copy link
Copy Markdown

@zuston zuston requested a review from advancedxy January 17, 2023 04:17
@advancedxy advancedxy self-assigned this Jan 17, 2023
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java Outdated
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java Outdated
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java Outdated
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java Outdated
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java Outdated
@zuston zuston requested a review from advancedxy January 17, 2023 10:32
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java Outdated
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java Outdated
Comment thread server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java Outdated
@zuston zuston requested a review from advancedxy January 19, 2023 03:13
}

public void markHugePartition(int shuffleId, int partitionId) {
if (!existHugePartition.get()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, I was thinking:

if (!existHugePartition.getAndSet(true)) {
        ShuffleServerMetrics.gaugeAppWithHugePartitionNum.inc();
        ShuffleServerMetrics.counterTotalAppWithHugePartitionNum.inc();
}

But maybe get is cheaper, and we should guide by it first.

@advancedxy

Copy link
Copy Markdown
Contributor

Generally lgtm, except two minor comments.

@advancedxy

Copy link
Copy Markdown
Contributor

LGTM, pending CI passes

@zuston zuston requested a review from advancedxy January 19, 2023 07:57
@zuston

zuston commented Jan 20, 2023

Copy link
Copy Markdown
Member Author

@advancedxy advancedxy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@zuston zuston merged commit 116125c into apache:master Jan 22, 2023
zuston pushed a commit that referenced this pull request Apr 26, 2024
### What changes were proposed in this pull request?

Fix the metric huge_partition_num.

### Why are the changes needed?

A follow-up PR for: #494.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing UTs.
roryqi pushed a commit that referenced this pull request Apr 30, 2024
### What changes were proposed in this pull request?

Fix the metric huge_partition_num.

### Why are the changes needed?

A follow-up PR for: #494.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing UTs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants