Skip to content

HDDS-1868. Ozone pipelines should be marked as ready only after the leader election is complete. - #23

Merged
nandakumar131 merged 14 commits into
apache:masterfrom
swagle:HDDS-1868
Nov 12, 2019
Merged

HDDS-1868. Ozone pipelines should be marked as ready only after the leader election is complete.#23
nandakumar131 merged 14 commits into
apache:masterfrom
swagle:HDDS-1868

Conversation

@swagle

@swagleswagle commented Oct 14, 2019

Copy link
Copy Markdown
Contributor

Ozone pipeline on create and restart, start in allocated state. They are moved into open state after all the pipeline have reported to it. However, this potentially can lead into an issue where the pipeline is still not ready to accept any incoming IO operations.

The pipelines should be marked as ready only after the leader election is complete and leader is ready to accept incoming IO.

mukul1987
mukul1987 requested changes Oct 16, 2019
View reviewed changes

@mukul1987mukul1987 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The patch looks good to me.

Comment thread...java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java Outdated
Comment thread...java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java Outdated
nandakumar131
nandakumar131 requested changes Oct 16, 2019
View reviewed changes

@nandakumar131nandakumar131 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall the change looks ok.

Comment thread...-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManager.java Outdated
Comment thread...e/src/main/java/org/apache/hadoop/ozone/container/common/report/PipelineReportPublisher.java Outdated
Comment thread...java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java Outdated
@swagle

swagle commented Oct 16, 2019

Copy link
Copy Markdown
ContributorAuthor

Thank you @mukul1987 and @nandakumar131 for the reviews.

@mukul1987

mukul1987 commented Oct 18, 2019

Copy link
Copy Markdown
Contributor

Thanks for updating the patch @swagle. There are some checkstyle issues in the latest patch and there are some test failures regarding pipeline esp along with the safe mode pipeline rules. Can you please take a look at the failures.

@mukul1987mukul1987 mentioned this pull request Oct 20, 2019
Merged
mukul1987
mukul1987 requested changes Oct 20, 2019
View reviewed changes
Comment threadhadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java Outdated
Comment thread...java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java Outdated
Comment thread...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineReportHandler.java Outdated
@swagle

swagle commented Oct 21, 2019

Copy link
Copy Markdown
ContributorAuthor

/retest

@vivekratnavel

vivekratnavel commented Oct 23, 2019

Copy link
Copy Markdown
Contributor

/retest

1 similar comment
@elek

elek commented Oct 23, 2019

Copy link
Copy Markdown
Member

/retest

nandakumar131
nandakumar131 reviewed Nov 6, 2019
View reviewed changes
Comment threadhadoop-hdds/common/src/main/proto/hdds.proto Outdated
Comment on lines 70 to 76

@nandakumar131nandakumar131Oct 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is no real change here, can we revert it?

@swagleswagleNov 6, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The leaderID was marked as optional.

Comment threadhadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java Outdated

@nandakumar131nandakumar131Nov 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: We can make it strongly typed by making the type as DatanodeDetails

@swagleswagleNov 6, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

But if I do that, in order to construct Pipeline from proto, I have to lookup DatanodeDetails based on UUID, which is a map in NodeManager. Not sure why making this into DatanodeDetails is better?

@nandakumar131nandakumar131Nov 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is just that it will be explicit what this UUID represent.

Comment thread...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineReportHandler.java Outdated
Comment thread...op-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/SCMPipelineManager.java Outdated
@nandakumar131

nandakumar131 commented Nov 11, 2019

Copy link
Copy Markdown
Contributor

/retest

@nandakumar131

nandakumar131 commented Nov 11, 2019

Copy link
Copy Markdown
Contributor

We can commit this after the Jenkins run.

@nandakumar131

nandakumar131 commented Nov 12, 2019

Copy link
Copy Markdown
Contributor

Test failure seems unrelated, merging this.

@nandakumar131
nandakumar131 merged commit ee8b89a into apache:masterNov 12, 2019
@timmylichengtimmylicheng mentioned this pull request Nov 15, 2019
Merged
kuenishi added a commit to pfnet/ozone that referenced this pull request Feb 22, 2022
@kuenishi
Bump version for new release (apache#23)
593811a
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mukul1987mukul1987mukul1987 requested changes
@nandakumar131nandakumar131nandakumar131 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@swagle@mukul1987@vivekratnavel@elek@nandakumar131