Skip to content

Conversation

@zuston
Copy link
Member

@zuston zuston commented Jul 21, 2022

What changes were proposed in this pull request?

Log enhancement: return error message when getting assignment servers and log exception when initializing

Why are the changes needed?

To be easy to find the cause of error.

Does this PR introduce any user-facing change?

No

How was this patch tested?

No need.

@jerqi
Copy link
Contributor

jerqi commented Jul 22, 2022

Is this pr a compatible improvement?

this.coordinatorConf = coordinatorConf;
initialization();

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hope all initializing exception message can be recorded into log files. Sometimes due to abnormal configuration, server or coordinator will exit directly no any log recorded, which is hard to debug.

Copy link
Contributor

Choose a reason for hiding this comment

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

There will be error msg in the stdout file when the server occur exception. But it's also acceptable change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. stdout message will lost when using ansible to deploy uniffle cluster.

@zuston
Copy link
Member Author

zuston commented Jul 22, 2022

Is this pr a compatible improvement?

I think it's a compatible chang.

@jerqi
Copy link
Contributor

jerqi commented Jul 22, 2022

Is this pr a compatible improvement?

I think it's a compatible chang.

Could you test the compatibility? Maybe we need auto compatibility test in our project.

@zuston
Copy link
Member Author

zuston commented Jul 26, 2022

Tested, it's OK @jerqi

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.

I verify the compatibility again. It's ok. LGTM. Thanks @zuston

@jerqi jerqi merged commit 20d39e8 into apache:master Jul 26, 2022
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.

2 participants