HDDS-1480. Prefer resolved datanode ip address over persisted ip address #7495
Conversation
ptlrs
changed the title
ptlrs
force-pushed
the
HDDS-1480-remove-ipaddress
branch
from
fe2316f to
85546fa
Compare
ptlrs
force-pushed
the
HDDS-1480-remove-ipaddress
branch
from
85546fa to
f0c156d
Compare
…tanode ip address
ptlrs
force-pushed
the
HDDS-1480-remove-ipaddress
branch
from
3f0d40f to
e35572c
Compare
|
@ptlrs, please avoid unnecessary formatting changes to the code, this makes it hard to review the logical changes done in the PR. |
There was a problem hiding this comment.
@ptlrs Thanks for the patch. The changes look good to me, excepting for some nitpicky changes. LGTM
...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java
Outdated
Show resolved
Hide resolved
| if (!path.exists()) { | ||
| throw new IOException("Datanode ID file not found."); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fixed. Although I do prefer to add space around if-clauses and in some places to have a logical grouping of statements.
...rvice/src/test/java/org/apache/hadoop/ozone/container/common/helpers/TestContainerUtils.java
Outdated
Show resolved
Hide resolved
|
@nandakumar131 Do you want to take another look at the patch? |
| try { | ||
| InetAddress inetAddress = InetAddress.getByName(datanodeDetails.getHostName()); | ||
|
|
||
| if (!inetAddress.getHostAddress().equals(datanodeDetails.getIpAddress())) { | ||
| LOG.warn("Resolved IP address '{}' differs from the persisted IP address '{}' for the datanode '{}'", | ||
| inetAddress.getHostAddress(), datanodeDetails.getIpAddress(), datanodeDetails.getHostName()); | ||
| datanodeDetails.setIpAddress(inetAddress.getHostAddress()); | ||
| } | ||
| } catch (UnknownHostException e) { | ||
| LOG.warn("Failed to validate IP address for the datanode '{}'", datanodeDetails.getHostName(), e); | ||
| } |
There was a problem hiding this comment.
Please extract the new logic to a separate method (preferably in DatanodeDetails). Previously readDatanodeDetailsFrom(File) did what its name said.
| private void assertWriteReadWithChangedIpAddress(@TempDir File tempDir, | ||
| DatanodeDetails details) throws IOException { |
There was a problem hiding this comment.
nit: Please do not format method signature like this. Whenever visibility / return type / method name / other modifiers are changed, we would have to reindent all parameters.
There was a problem hiding this comment.
@adoroszlai Can you please point me to any existing methods which follow the preferred format for my reference.
There was a problem hiding this comment.
I don't think there is any strong preference we agreed on, but you can't go wrong by following existing style from the same file being edited:
Some would argue for:
- putting each argument on a separate line vs. grouping few of them while respecting line length limit
- putting the first argument on its own line
Consistent formatting has benefits, but coming to agreement is not easy, and reformatting all existing code makes backports harder initially.
| File file = new File(tempDir, "valid-values.id"); | ||
| ContainerUtils.writeDatanodeDetailsTo(details, file, conf); | ||
| DatanodeDetails read = ContainerUtils.readDatanodeDetailsFrom(file); | ||
| assertNotEquals(details.toString(), read.toString()); |
There was a problem hiding this comment.
Can you please add a bit stronger assertion about the IP address?
...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java
Outdated
Show resolved
Hide resolved
…ozone/container/common/helpers/ContainerUtils.java Co-authored-by: Siyao Meng <[email protected]>
Please describe your PR in detail:
DatanodeDetailsis persisted byContainerUtilsandDatanodeIdYamlclasses.The IP address in
DatanodeDetailsis also persisted.However, the IP address could have changed when reading back the persisted file.
The original task suggests removal of the IP address field.
Currently the IP address field of the
DatanodeDetailsclass is used in several places.So removal of the IP address field either from persistence or from usage is not straightforward. As an alternative, we resolve the IP address based on the hostname to detect if the IP address has changed.
As part of this change:
UnknownHostExceptionWhat is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-1480
How was this patch tested?
CI: https://github.com/ptlrs/ozone/actions/runs/12135607178https://github.com/ptlrs/ozone/actions/runs/12627875515