-
Notifications
You must be signed in to change notification settings - Fork 326
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
Add address and pid prefix to the mars exception message #2730
Add address and pid prefix to the mars exception message #2730
Conversation
mars/oscar/backends/message.py
Outdated
class _MarsError(ErrorMessage.AsCauseBase, type(self.error)): | ||
pass | ||
|
||
return _MarsError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapping existing errors has some side effects. For instance, some exception types have custom attributes, for instance, HTTP error codes. Wrapping with error messages hide these attributes and may lead to unexpected errors. Simply try adding attributes to these exceptions and log locations when handling exceptions at receiver end may be a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks. I added a test case test_as_instanceof_cause
for testing as_instanceof_cause
.
mars/oscar/backends/pool.py
Outdated
@@ -59,8 +59,9 @@ | |||
|
|||
|
|||
class _ErrorProcessor: | |||
def __init__(self, message_id: bytes, protocol): | |||
def __init__(self, message_id: bytes, address: str, protocol): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply use pool
as the argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the _ErrorProcessor
needs some message info and the address of current pool. Replacing address
to pool
does not simplify the constructor parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ars exception message (mars-project#2730) Merge branch improve_mars_exception_compatibility of git@gitlab.alipay-inc.com:ray-project/mars.git into master https://code.alipay.com/ray-project/mars/pull_requests/251 Signed-off-by: 慕白 <chaokun.yck@antgroup.com> * Add address and pid prefix to the mars exception message (mars-project#2730) * Fix CI
Merge branch cp_2633_2723_2730 of git@gitlab.alipay-inc.com:ray-project/mars.git into master https://code.alipay.com/ray-project/mars/pull_requests/266 Signed-off-by: 不涸 <zhongchun.yzc@antgroup.com> * Refine failure recovery log and exception (mars-project#2633) * Refine fo log and exception * Pin xgboost_ray to 0.1.5 Co-authored-by: 留宝 <po.lb@antgroup.com> Co-authored-by: 刘宝 <po.lb@antfin.com> * Fix duplicate exceptions in log (mars-project#2723) * Add address and pid prefix to the mars exception message (mars-project#2730)
What do these changes do?
Developers need the address and pid for debug in a large distributed cluster, so this PR,
An example exception,
Related issue number
Fixes #xxxx
Check code requirements