Skip to content

Conversation

@dombizita
Copy link
Contributor

@dombizita dombizita commented Mar 9, 2023

What changes were proposed in this pull request?

previous this change the DatanodeDetails didn't store any information regarding the HTTP and HTTPS ports of the datanode. there was a conversation on how to add the ports in a backward compatible way and we decided to add a new HDDS layout version as WEBUI_PORTS_IN_DATANODEDETAILS. the original task got separated into two more tasks that was done prior this. one to handle the the 1.4.0 upgrade test, which got covered by #4456 and one to add a layout version for the RATIS_DATASTREAM port, which was done in #4500.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7817

How was this patch tested?

green CI on my fork: https://github.com/dombizita/ozone/actions/runs/4369081456/jobs/7650370829

dombizita added 10 commits March 6, 2023 11:44
Change-Id: Ie6fe9a79a5b70c83d42b43de7b047cd8f020cad2
Change-Id: I0d99b9a525958737a942f8172ddd9782f7465cac
Change-Id: Ie07034fd0c458e8a0be5dc847b99611120b8ec42
Change-Id: Ia3b9f514709f6d8582ae1dfc90f6f8854813c945
Change-Id: If123a03ab79e9deea4a6f9a7d492b446377dfb57
Change-Id: If2bcebe5e06cb27d17c577d26703487e4a0475bb
Change-Id: Ic5f8151e53414e16755a43e69a8c37f0b195e5f8
Change-Id: Id56cf432d85ff1863fb01c703c567fd888c1877e
…version anymore

Change-Id: If5bc19fcdd158105c1f9c0e3840dbe3c5440b8a8
Change-Id: Ic478148742dffaeeaf507cefe83b8c43eaac525d
@dombizita
Copy link
Contributor Author

please take a look at it @fapifta @adoroszlai @errose28

@fapifta fapifta requested a review from errose28 March 9, 2023 12:41
Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

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

Thank you @dombizita for working on this piece.

I have one minor comment inline.
Besides that I think it would be useful to create a test which checks the case where the metadata layout version in the DatanodeLayoutStorage managed VERSION file is still under 5, and check if the createDatanodeIdFile call writes just the expected ports, while if it is already 5, then it writes the HTTP/HTTPS ports as well, would you mind add these tests?

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @dombizita for working on this.

I agree with @fapifta about the need for adding a unit test (in TestDatanodeIdYaml).

(Note: please update from your fork, I've merged master into your branch and resolved some conflicts caused by my latest commit.)

Change-Id: I0f8ec522d35ee66f383ee2c87d0418b1fc9e34a9
Change-Id: I49ed8a1e7bd54d81cedc4c28e1eb4df5b690cd1c
@fapifta
Copy link
Contributor

fapifta commented Mar 10, 2023

I have restarted the failed test as it seemed an irrelevant failure (SCM stuck in safe mode after successfully exiting a couple of times already).

I am +1 for the changes to be committed, but I would like to see if @errose28 can also review, and how @adoroszlai sees my opinion regarding the RATIS_DATASTREAM port.

@dombizita dombizita marked this pull request as draft March 22, 2023 13:07
Change-Id: Ic9ef1f0e86c2c44854ec8c9ed1264d88faa9ad29
@dombizita dombizita marked this pull request as ready for review March 31, 2023 13:44
Change-Id: I95fcd033f4c40f76b257768e22189839fd07967b
@dombizita dombizita requested review from adoroszlai and fapifta March 31, 2023 13:57
@adoroszlai adoroszlai merged commit 3799bb2 into apache:master Apr 1, 2023
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.

4 participants