Skip to content

[CELEBORN-1821][CIP-14] Add controlMessages to cppClient#3052

Closed
HolyLow wants to merge 2 commits intoapache:mainfrom
HolyLow:issue/celeborn-1821-add-control-messages-to-cpp-client
Closed

[CELEBORN-1821][CIP-14] Add controlMessages to cppClient#3052
HolyLow wants to merge 2 commits intoapache:mainfrom
HolyLow:issue/celeborn-1821-add-control-messages-to-cpp-client

Conversation

@HolyLow
Copy link
Copy Markdown
Contributor

@HolyLow HolyLow commented Jan 6, 2025

What changes were proposed in this pull request?

This PR adds ControlMessages to cppClient.

Why are the changes needed?

The ControlMessages are used to communicate with the CelebornServer and LifecycleManager.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Compilation and UTs.

@HolyLow
Copy link
Copy Markdown
Contributor Author

HolyLow commented Jan 6, 2025

@FMX @RexXiong Could you kindly help review this PR? Thanks a lot.

Any suggestion is welcome.

auto fileGroups = pbGetReducerFileGroupResponse->filegroups();
for (auto& kv : fileGroups) {
auto& fileGroup = response->fileGroups[kv.first];
// Legacy mode: the locations are valid, so use it.
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.

shall we need support legacy mode?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In java end, the legacy mode is kept for backward compatibility. I think keeping legacy mode in CppClient does no harm, or should we simply throw an error here if the legacy mode is detected?
Any suggestion is welcome.

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.

Native SDK will be shipped in 0.6 or 0.7. Should use new format as default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactored as requested.
Any suggestion is welcome.

Copy link
Copy Markdown
Contributor

@FMX FMX left a comment

Choose a reason for hiding this comment

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

LGTM.

@FMX
Copy link
Copy Markdown
Contributor

FMX commented Jan 8, 2025

Merged into main(v0.6.0).

@FMX FMX closed this in b74e05b Jan 8, 2025
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