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-1368] Log celeborn config for debugging purposes #2442

Closed
wants to merge 7 commits into from

Conversation

akpatnam25
Copy link
Contributor

@akpatnam25 akpatnam25 commented Apr 3, 2024

What changes were proposed in this pull request?

Log celeborn config for debugging purposes.

Why are the changes needed?

Help with debugging

Does this PR introduce any user-facing change?

No

How was this patch tested?

tested the patch internally.

@akpatnam25
Copy link
Contributor Author

cc @waitinfuture @SteNicholas @FMX can you guys please take a look? thanks.

@SteNicholas
Copy link
Member

@akpatnam25, why does this need to log celeborn config? Could user use /conf REST API to get the config? BTW, if this need to log celeborn log, you could invoke HttpService#getConf method to log.

@akpatnam25
Copy link
Contributor Author

akpatnam25 commented Apr 3, 2024

@akpatnam25, why does this need to log celeborn config? Could user use /conf REST API to get the config? BTW, if this need to log celeborn log, you could invoke HttpService#getConf method to log.

We do not have the REST api changes internally yet, as we are running with 0.4 still. This would unblock us quick, in the long term I agree we should use the /conf REST API. I will use the HttpService#getConf, I did not know it existed.

@cxzl25 cxzl25 changed the title CELEBORN-1368: Log celeborn config for debugging purposes [CELEBORN-1368] Log celeborn config for debugging purposes Apr 5, 2024
@waitinfuture
Copy link
Contributor

Hi @akpatnam25 , please run UPDATE=1 build/mvn clean test -pl common -am -Pspark-3.3 -Dtest=none -DwildcardSuites=org.apache.celeborn.ConfigurationSuite to update the doc since configs changed in this PR.

Copy link
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.

I think this PR can be changed.

@akpatnam25
Copy link
Contributor Author

@SteNicholas @pan3793 @FMX @waitinfuture I have updated this PR. please take another look, thanks

@FMX
Copy link
Contributor

FMX commented Apr 8, 2024

@akpatnam25 There is a compilation error that needs to be fixed first.
截屏2024-04-08 14 40 36

@akpatnam25
Copy link
Contributor Author

@akpatnam25 There is a compilation error that needs to be fixed first. 截屏2024-04-08 14 40 36

I accidentally deleted a line! Should be fixed now. @FMX

Copy link
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. Thanks. Merged into main(v0.5.0).

@FMX FMX closed this in f04ebcc Apr 8, 2024
@akpatnam25
Copy link
Contributor Author

thanks for the reviews @SteNicholas @FMX @pan3793 @waitinfuture !!

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.

5 participants