Skip to content

[GLUTEN-10107][CH] Decouple Celeborn-related code from CH backend module#10537

Merged
philo-he merged 6 commits intoapache:mainfrom
zjuwangg:m_refactorCHRss
Aug 29, 2025
Merged

[GLUTEN-10107][CH] Decouple Celeborn-related code from CH backend module#10537
philo-he merged 6 commits intoapache:mainfrom
zjuwangg:m_refactorCHRss

Conversation

@zjuwangg
Copy link
Copy Markdown
Contributor

@zjuwangg zjuwangg commented Aug 26, 2025 •
edited
Loading

What changes are proposed in this pull request?

As a follow up of #10201, aims to complete #10107

This MR mainly focus on decouple celeborn config from ch backend.

How was this patch tested?

Existing test.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 26, 2025

Run Gluten Clickhouse CI on x86

@zjuwangg zjuwangg changed the title [wip]decouple celeborn related config from ch backend [GLUEN-10107]decouple celeborn related config from ch backend Aug 26, 2025
@zjuwangg zjuwangg changed the title [GLUEN-10107]decouple celeborn related config from ch backend [GLUEN-10107][CH]decouple celeborn related config from ch backend Aug 26, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 26, 2025

Run Gluten Clickhouse CI on x86

@philo-he philo-he changed the title [GLUEN-10107][CH]decouple celeborn related config from ch backend [GLUEN-10107][CH] Decouple celeborn related config from ch backend Aug 26, 2025
@zjuwangg
Copy link
Copy Markdown
Contributor Author

zjuwangg commented Aug 27, 2025

I noticed there exist a ci failure but I can't see the details. Can anyone help provide the error log?
@philo-he @zhztheplayer

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 27, 2025

Run Gluten Clickhouse CI on x86

@philo-he
Copy link
Copy Markdown
Member

philo-he commented Aug 27, 2025

Is there any error message reported from the following?

https://github.com/apache/incubator-gluten/blob/d91058634842f8d34de81c00c68e8f10e4c1c7b3/docs/get-started/ClickHouse.md?plain=1#L673-L674

@zjuwangg
Copy link
Copy Markdown
Contributor Author

zjuwangg commented Aug 27, 2025

Is there any error message reported from the following?

https://github.com/apache/incubator-gluten/blob/d91058634842f8d34de81c00c68e8f10e4c1c7b3/docs/get-started/ClickHouse.md?plain=1#L673-L674

Sorry, I missed account info here.
Will address the ci failure soon, thx a lot.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 27, 2025

Run Gluten Clickhouse CI on x86

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 27, 2025

Run Gluten Clickhouse CI on x86

@zjuwangg
Copy link
Copy Markdown
Contributor Author

zjuwangg commented Aug 28, 2025

@zhztheplayer Would you like to help review this modification,too? thx a lot

philo-he
philo-he reviewed Aug 28, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@philo-he philo-he left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I note some typos. Please help fix them. Thanks.

@github-actions github-actions bot added the VELOX label Aug 28, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 28, 2025

Run Gluten Clickhouse CI on x86

@zhouyuan zhouyuan changed the title [GLUEN-10107][CH] Decouple celeborn related config from ch backend [GLUTEN-10107][CH] Decouple celeborn related config from ch backend Aug 28, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 28, 2025

#10107

@zhouyuan
Copy link
Copy Markdown
Member

zhouyuan commented Aug 28, 2025

Cc: @baibaichen @zzcclp

philo-he
philo-he reviewed Aug 28, 2025
View reviewed changes
...-celeborn/src/main/java/org/apache/spark/shuffle/gluten/celeborn/CelebornShuffleManager.java Outdated
// backendName);
// }
columnarBatchSerializerFactory = columanrBatchSerilizerFactoryMap.get(backendName);
if (!columnarBatchSerilizerFactoryMap.containsKey(backendName)) {
Copy link
Copy Markdown
Member

@philo-he philo-he Aug 28, 2025

Choose a reason for hiding this comment

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

Not directly related to this PR's target.

I think using a map might be unnecessary. As we know, the implementation for ColumnarBatchSerializerFactory is exclusive, meaning Velox's factory implementation can only be loaded by the Velox backend, and CH's implementation can only be loaded by the CH backend. Then, it seems we can just check whether a concrete factory is loaded. There's no need to store the backend name in the factory or check the factory's backend name to confirm it belongs to the current backend.

This is just a preliminary thought. Am I missing something?

Copy link
Copy Markdown
Contributor Author

@zjuwangg zjuwangg Aug 29, 2025

Choose a reason for hiding this comment

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

I think you are right. Just open a issue to track this insightful idea #10577

@zhztheplayer
Copy link
Copy Markdown
Member

zhztheplayer commented Aug 28, 2025

No major concern from my end. Thanks @zjuwangg. Please follow up with @philo-he for the previous reviewing.

Look forward to finally having the modules fully decoupled even without the service registers.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 29, 2025

Run Gluten Clickhouse CI on x86

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 29, 2025

Run Gluten Clickhouse CI on x86

@zjuwangg zjuwangg mentioned this pull request Aug 29, 2025
Closed
@philo-he philo-he changed the title [GLUTEN-10107][CH] Decouple celeborn related config from ch backend [GLUTEN-10107][CH] Decouple Celeborn-related code from CH backend module Aug 29, 2025
philo-he
philo-he approved these changes Aug 29, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@philo-he philo-he left a comment

Choose a reason for hiding this comment

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

Looks good. Will merge this PR today if no other comments from the community.

@philo-he philo-he merged commit d790141 into apache:main Aug 29, 2025
98 of 99 checks passed
@zjuwangg zjuwangg deleted the m_refactorCHRss branch September 1, 2025 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@philo-he philo-he philo-he approved these changes

Assignees

No one assigned

Labels

CLICKHOUSE RSS VELOX

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zjuwangg @philo-he @zhouyuan @zhztheplayer