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

[CELEBORN-1258] Add UserIdentifier into the master application meta #2296

Closed
wants to merge 4 commits into from

Conversation

turboFei
Copy link
Member

@turboFei turboFei commented Feb 17, 2024

What changes were proposed in this pull request?

Record the userIdentifier in master application meta.
Also including app totalWritten, fileCount info.

Why are the changes needed?

To close CELEBORN-1258

Does this PR introduce any user-facing change?

Yes.
The result format of master RESTful api /applications changed.

How was this patch tested?

UT.

@@ -611,6 +612,7 @@ message PbSnapshotMetaInfo {
repeated PbWorkerInfo shutdownWorkers = 13;
repeated PbWorkerInfo manuallyExcludedWorkers = 14;
map<string, PbWorkerEventInfo> workerEventInfos = 15;
map<string, PbApplicationInfo> applications = 16;
Copy link
Member Author

Choose a reason for hiding this comment

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

should we use a different field number with deprecated appHeartbeatTime?

  map<string, int64> appHeartbeatTime = 6;

Copy link
Member Author

Choose a reason for hiding this comment

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

@turboFei turboFei force-pushed the CELEBORN-1258_userId branch 2 times, most recently from 55b28fd to d5c3ed0 Compare February 17, 2024 22:10
@turboFei turboFei changed the title [CELEBORN-1258] Add UserIdentifier to the master application meta [CELEBORN-1258] Add UserIdentifier into the master application meta Feb 17, 2024
Copy link

codecov bot commented Feb 17, 2024

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (c183753) 48.63% compared to head (b16dbb2) 48.81%.
Report is 6 commits behind head on main.

❗ Current head b16dbb2 differs from pull request most recent head bffbb65. Consider uploading reports for the commit bffbb65 to get more accurate results

Files Patch % Lines
...org/apache/celeborn/common/util/PbSerDeUtils.scala 0.00% 12 Missing ⚠️
...born/common/protocol/message/ControlMessages.scala 0.00% 8 Missing ⚠️
.../apache/celeborn/common/meta/ApplicationInfo.scala 80.00% 2 Missing and 1 partial ⚠️
...ache/celeborn/common/identity/UserIdentifier.scala 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2296      +/-   ##
==========================================
+ Coverage   48.63%   48.81%   +0.18%     
==========================================
  Files         208      209       +1     
  Lines       12832    12869      +37     
  Branches     1104     1105       +1     
==========================================
+ Hits         6240     6281      +41     
+ Misses       6191     6187       -4     
  Partials      401      401              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SteNicholas
Copy link
Member

@turboFei, the UserIdentifier does not just simply add into ApplicationHeartbeater, which could be add into RegistrationClientBootstrap#register. We should firstly abstract the application registeration, then add UserIdentifier into the master application meta. cc @RexXiong.

@turboFei
Copy link
Member Author

@turboFei, the UserIdentifier does not just simply add into ApplicationHeartbeater, which could be add into RegistrationClientBootstrap#register. We should firstly abstract the application registeration, then add UserIdentifier into the master application meta. cc @RexXiong.

ok. thanks for the information.

@turboFei
Copy link
Member Author

turboFei commented Mar 7, 2024

Raised #2365, close this one.

@turboFei turboFei closed this Mar 7, 2024
@turboFei turboFei deleted the CELEBORN-1258_userId branch March 7, 2024 23:33
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