Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#1114] feat: introduce hdfs host as the total_hadoop_write_data metric label #1107

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

zuston
Copy link
Member

@zuston zuston commented Aug 7, 2023

What changes were proposed in this pull request?

Introduce the hdfs host as the total_hadoop_write_data metric label

Why are the changes needed?

We will write apps of different businesses apps into different HDFS.
For further iterative optimization, we need to expose the write capacity of
different HDFS to evaluate the total write capacity of different business lines.

Does this PR introduce any user-facing change?

Yes. More metrics are exposed.

How was this patch tested?

  1. UTs

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2023

Codecov Report

Merging #1107 (04134cd) into master (ef7f392) will increase coverage by 1.38%.
Report is 11 commits behind head on master.
The diff coverage is 58.33%.

@@             Coverage Diff              @@
##             master    #1107      +/-   ##
============================================
+ Coverage     54.10%   55.48%   +1.38%     
- Complexity     2547     2562      +15     
============================================
  Files           386      368      -18     
  Lines         21901    19635    -2266     
  Branches       1815     1826      +11     
============================================
- Hits          11850    10895     -955     
+ Misses         9352     8106    -1246     
+ Partials        699      634      -65     
Files Changed Coverage Δ
...e/uniffle/server/storage/HadoopStorageManager.java 87.67% <40.00%> (-3.64%) ⬇️
...rg/apache/uniffle/server/ShuffleServerMetrics.java 94.91% <71.42%> (-1.52%) ⬇️

... and 40 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jerqi
Copy link
Contributor

jerqi commented Aug 7, 2023

Should we create an issue for this pr?

@zuston zuston changed the title feat: introduce hdfs host as the total_hadoop_write_data metric label [#1114] feat: introduce hdfs host as the total_hadoop_write_data metric label Aug 8, 2023
@zuston
Copy link
Member Author

zuston commented Aug 8, 2023

Should we create an issue for this pr?

Done

@zuston
Copy link
Member Author

zuston commented Aug 8, 2023

ptal @jerqi

@jerqi
Copy link
Contributor

jerqi commented Aug 8, 2023

ptal @jerqi

I'm not familiar with label. Let @smallzhongfeng @xianjingfeng @advancedxy take a look.

@@ -125,7 +126,7 @@ public class ShuffleServerMetrics {
public static Counter.Child counterTotalReadTime;
public static Counter.Child counterTotalFailedWrittenEventNum;
public static Counter.Child counterTotalDroppedEventNum;
public static Counter.Child counterTotalHadoopWriteDataSize;
public static Counter counterTotalHadoopWriteDataSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

would you move this field to L162 - L165, and grouped together?

  public static Counter counterRemoteStorageTotalWrite;
  public static Counter counterRemoteStorageRetryWrite;
  public static Counter counterRemoteStorageFailedWrite;
  public static Counter counterRemoteStorageSuccessWrite;

@advancedxy
Copy link
Contributor

LGTM, except one minor comment.

smallzhongfeng
smallzhongfeng previously approved these changes Aug 9, 2023
Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM.

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.

None yet

5 participants