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

Fix invalid parameter issue when force to close endpoint #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brianplus
Copy link

What

Fix invalid parameter exception when force to close endpoint.

Why ?

When UcxWorkerWrapper forces close the endpoints, it will hit org.openucx.jucx.UcxException: Invalid parameter issue. It's because in UCX native code, ucp_ep_close_nbx, when UCP_EP_CLOSE_FLAG_FORCE is set, UCP_ERR_HANDLING_MODE_PEER also has to be set to key's err_mode.

How ?

Call setPeerErrorHandlingMode to set UCP_ERR_HANDLING_MODE_PEER when create endpoint connection and call setErrorHandler to set error handling code as well.

@brianplus
Copy link
Author

@petro-rudenko could you help to review the code change? Thanks.

@@ -188,7 +188,14 @@ case class UcxWorkerWrapper(worker: UcpWorker, transport: UcxShuffleTransport, i
def connectByWorkerAddress(executorId: transport.ExecutorId, workerAddress: ByteBuffer): Unit = {
logDebug(s"Worker $this connecting back to $executorId by worker address")
val ep = worker.newEndpoint(new UcpEndpointParams().setName(s"Server connection to $executorId")
.setUcpAddress(workerAddress))
.setUcpAddress(workerAddress)
.setPeerErrorHandlingMode()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yosefe does peerErrorHandling mode works with connect by worker address?

Choose a reason for hiding this comment

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

@yosefe does peerErrorHandling mode works with connect by worker address?

@yosefe Can you please review? Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

In UCX,

ucs_status_ptr_t ucp_ep_close_nbx(ucp_ep_h ep, const ucp_request_param_t *param)
{
    ucp_worker_h  worker = ep->worker;
    void          *request = NULL;
    ucp_request_t *close_req;

    if ((ucp_request_param_flags(param) & UCP_EP_CLOSE_FLAG_FORCE) &&
        (ucp_ep_config(ep)->key.err_mode != UCP_ERR_HANDLING_MODE_PEER)) {
        return UCS_STATUS_PTR(UCS_ERR_INVALID_PARAM);
    }

when force to close the ep, it will also check the flag UCP_ERR_HANDLING_MODE_PEER setting.

Choose a reason for hiding this comment

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

@gleon99 @yosefe Can you pls help to review and merge this PR? Thanks!

@petro-rudenko
Copy link
Collaborator

Can you please sign your commit

@brianplus brianplus force-pushed the dev/brian/closeNonBlockingForce_invalid_parameter branch from 31ac160 to 4cdaece Compare March 10, 2023 09:09
jeynmann referenced this pull request in jeynmann/sparkucx Mar 17, 2023
@brianplus
Copy link
Author

Signed. Thanks Peter.

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