HDDS-3498. Shutdown datanode if address is already in use #7256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Daniilchik thank you for working on this. |
|
@sarvekshayr After |
ivanzlenko
suggested changes
Oct 1, 2024
| "Unable to communicate to {} server at {} for past {} seconds.", | ||
| serverName, | ||
| getAddress().getHostString() + ":" + getAddress().getPort(), | ||
| address.getAddress() + ":" + address.getPort(), |
There was a problem hiding this comment.
Please use log4j templates instead of concatenation.
| @@ -108,6 +109,10 @@ public EndpointStateMachine.EndPointStates call() throws Exception { | |||
| rpcEndPoint.setState(EndpointStateMachine.EndPointStates.SHUTDOWN); | |||
| } catch (IOException ex) { | |||
| rpcEndPoint.logIfNeeded(ex); | |||
| if (ex.getCause() instanceof NativeIoException) { | |||
| LOG.error(ex.getMessage()); | |||
There was a problem hiding this comment.
Shouldn't we log stacktrace here?
myskov
requested changes
Oct 1, 2024
There was a problem hiding this comment.
Thanks @Daniilchik for working on the patch.
A few thoughts:
NativeIoException.classshould not be exposed toVersionEndpointTask, as it is an internal detail of OzoneContainer. I believeOzoneContainer.start()should throw a dedicated exception for this scenario, perhaps something likeContainerStartException.rpcEndPoint.logIfNeeded(ex)should not be called in the event of a port binding error. This call is responsible for the log message 'Unable to communicate with SCM server at scm/172.20.0.3:9861', but that’s not relevant in this situation.ozoneContainer.stop()should not be invoked directly. Simply callingrpcEndPoint.setState(EndpointStateMachine.EndPointStates.SHUTDOWN)is sufficient to initiate the shutdown process.
|
@myskov thanks for help, all done. |
adoroszlai
changed the title
Daniilchik
force-pushed
the
HDDS-3498
branch
2 times, most recently
from
8c47c15
to
c151608
Compare
|
@Daniilchik please do not use |
Sorry about that, I won’t do it again. |
sarvekshayr
reviewed
Oct 2, 2024
ivanzlenko
suggested changes
Oct 2, 2024
| @@ -107,7 +107,8 @@ public EndpointStateMachine.EndPointStates call() throws Exception { | |||
| } catch (DiskOutOfSpaceException ex) { | |||
| rpcEndPoint.setState(EndpointStateMachine.EndPointStates.SHUTDOWN); | |||
| } catch (IOException ex) { | |||
| rpcEndPoint.logIfNeeded(ex); | |||
| LOG.error(ex.getCause().getMessage(), ex); | |||
There was a problem hiding this comment.
LOG.error(ex) should already print message from exception. I would rather put here thoughtful and distinct message so it will be much easier to pinpoint place of the failure and understand when and why it failed.
ivanzlenko
reviewed
Oct 2, 2024
| server.start(); | ||
| } catch (IOException e) { | ||
| LOG.error("Failed to bind to address", e); | ||
| if (e.getMessage().contains("Failed to bind to address")) { |
There was a problem hiding this comment.
Do we have any actual reason to start a service if we can't start a server? Shouldn't we just handle all IOException like that?
There was a problem hiding this comment.
If we will handle all IOException like that we will end up with shutting down datanodes due to any network failures.
There was a problem hiding this comment.
Can we identify how actual retryable exceptions looks like? Is netty throwing specifically only IOExceptions here?
There was a problem hiding this comment.
Actually it is better just handle correctly server.start part and shutdown everything here cause we can't start server. There is no point to live afterwards.
There was a problem hiding this comment.
Discussed it with Daniil - we need to return to this code at some point in time to refactor it.
ivanzlenko
suggested changes
Oct 2, 2024
| try { | ||
| server.start(); | ||
| } catch (IOException e) { | ||
| LOG.error("Failed to bind to address", e); |
There was a problem hiding this comment.
Failed to bind to address - you are writing this text for any IOException. What if it was other issue preventing us from starting server?
Message something like "Can't start whatever server due to error occurred during startup process:" will be more appropriate here.
ivanzlenko
suggested changes
Oct 2, 2024
| serverName, | ||
| address.getAddress(), | ||
| address.getPort(), | ||
| TimeUnit.MILLISECONDS.toSeconds(this.getMissedCount() * getScmHeartbeatInterval(this.conf)), |
There was a problem hiding this comment.
I would rather calculate it beforehand in a dedicated variable just to improve code readability.
ERROR server.XceiverServerGrpc: Error while starting the server |
myskov
reviewed
Oct 3, 2024
...ntainer-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/BindException.java
Outdated
Show resolved
Hide resolved
myskov
requested changes
Oct 3, 2024
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/common/TestEndPoint.java
Outdated
Show resolved
Hide resolved
myskov
reviewed
Oct 4, 2024
...c/main/java/org/apache/hadoop/ozone/container/common/transport/server/XceiverServerGrpc.java
Outdated
Show resolved
Hide resolved
myskov
approved these changes
Oct 4, 2024
There was a problem hiding this comment.
The most recent version looks good to me. @ivanzlenko please take a look.
ivanzlenko
reviewed
Oct 4, 2024
| long missedDurationSeconds = TimeUnit.MILLISECONDS.toSeconds( | ||
| this.getMissedCount() * getScmHeartbeatInterval(this.conf) | ||
| ); | ||
| LOG.error( |
There was a problem hiding this comment.
Why you changed it to error?
There was a problem hiding this comment.
Thank you for the remark. I’ll change it back. I initially changed it at the beginning of working on the task.
| server.start(); | ||
| } catch (IOException e) { | ||
| LOG.error("Failed to bind to address", e); | ||
| if (e.getMessage().contains("Failed to bind to address")) { |
There was a problem hiding this comment.
Discussed it with Daniil - we need to return to this code at some point in time to refactor it.
ivanzlenko
approved these changes
Oct 4, 2024
|
@Daniilchik thank you for working on the patch. Thanks @sarvekshayr and @ivanzlenko for reviewing. |
sarvekshayr
pushed a commit
to sarvekshayr/ozone
that referenced
this pull request
Oct 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR fixes a bug related to an incomplete datanode process.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3498
How was this patch tested?
Manual tests using docker-compose, build-branch workflow.