Skip to content

HDDS-8024. When readChunk from a datanode fails, retry other datanodes. #4336

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.

Sign up for GitHub

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

Jump to bottom
Merged
szetszwo merged 2 commits into from
Mar 4, 2023
Merged

HDDS-8024. When readChunk from a datanode fails, retry other datanodes. #4336

szetszwo merged 2 commits into from
Mar 4, 2023

Conversation

@szetszwo
Copy link
Contributor

@szetszwo szetszwo commented Mar 1, 2023

What changes were proposed in this pull request?

When failing read chunk from a datanode, exclude it and retry other datanodes.

What is the link to the Apache JIRA

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

How was this patch tested?

Updated TestHSync

jojochuang
jojochuang approved these changes Mar 4, 2023
View reviewed changes
Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

Thanks. This is a good fix and looks good to me!

Prior to this change, BlockInputStream defines retry policy which retries three times with a 1 ms delay before retry.
After this change, reading a block could retry up to 9 times (assuming 3 replicas): r1, r2, r3, r1, r2, r3, r1, r2, r3.

It might be a good idea to extend the delay between retrying r3 and r1, in case of cluster-wide restart. In fact, we should consider testing such cases so that clients don't fail prematurely.

Also for the case where block token expires (https://issues.apache.org/jira/browse/HDDS-7930) trying another replica will not help, although the retry policy at BlockInputStream level will address this, and a client is supposed to only hit the block token expiration rarely.

Just a suggestion: can you update the jira subject so that it is easier for the community to tell this is a behavior change, not just fixing flaky tests?

Other than that I am +1.

@szetszwo szetszwo changed the title HDDS-8024. Intermittent inconsistent read in HSync tests. HDDS-8024. When readChunk from a datanode fails, retry other datanodes. Mar 4, 2023
@szetszwo szetszwo merged commit 3bbb574 into apache:master Mar 4, 2023
@szetszwo
Copy link
Contributor Author

szetszwo commented Mar 4, 2023

@jojochuang , thanks a lot for reviewing this! Updated the title.

@adoroszlai
Copy link
Contributor

adoroszlai commented Mar 4, 2023

@szetszwo thanks for the improvement. Sorry for being late, but I tried this fix in repeated CI runs and it didn't seem to fix the problem.

56 out of 300 iterations failed:
https://github.com/adoroszlai/hadoop-ozone/actions/runs/4326162668

@szetszwo
Copy link
Contributor Author

szetszwo commented Mar 4, 2023

@adoroszlai , thanks a lot for testing it!

	at java.io.DataInputStream.read(DataInputStream.java:149)
	at org.apache.hadoop.fs.ozone.TestHSync.runTestHSync(TestHSync.java:201)
	at org.apache.hadoop.fs.ozone.TestHSync.runTestHSync(TestHSync.java:154)
	at org.apache.hadoop.fs.ozone.TestHSync.testO3fsHSync(TestHSync.java:122)

https://github.com/adoroszlai/hadoop-ozone/actions/runs/4326162668/jobs/7553254946#step:5:6237

The line numbers do not match the latest code. I guess it was testing the old code?

@adoroszlai
Copy link
Contributor

adoroszlai commented Mar 5, 2023

The line numbers do not match the latest code. I guess it was testing the old code?

No, it was testing the PR, you can find diff here: master...adoroszlai:hadoop-ozone:HDDS-8024-repeat

Line numbers in TestHSync are off because a01676a changed that file on master in the meantime.

@szetszwo
Copy link
Contributor Author

szetszwo commented Mar 6, 2023

Line numbers in TestHSync are off because a01676a changed that file on master in the meantime.

@adoroszlai , a01676a is HDDS-8029 which is BEFORE this commit 3bbb574 as shown below. Anyway, could you run the test again with the latest code?

commit 3bbb5742c5f494d940ced1553e5772eecfb6398c
Author: Tsz-Wo Nicholas Sze <[email protected]>
Date:   Fri Mar 3 17:03:45 2023 -0800

    HDDS-8024. When readChunk from a datanode fails, retry other datanodes. (#4336)

commit 88292e8a446d183d78f74aefda2b72d3e02ab2f7
Author: Stephen O'Donnell <[email protected]>
Date:   Fri Mar 3 21:00:45 2023 +0000

    HDDS-8075. ECReconstructionCoordinatorTask.runTask should catch Exception (#4342)

commit a432438c363622d287fb4ac3524394fa2ec5fc13
Author: djordje-mijatovic <[email protected]>
Date:   Fri Mar 3 21:33:47 2023 +0100

    HDDS-7210. Missing open containers show up as "Closing" on the container report. (#4207)

commit a01676a9afccf4d7c4c80066d999a087973a9def
Author: Wei-Chiu Chuang <[email protected]>
Date:   Fri Mar 3 11:14:41 2023 -0800

    HDDS-8029. [hsync] Outputstream in encrypted buckets do not return the correct stream capabilities. (#4316)

@adoroszlai
Copy link
Contributor

adoroszlai commented Mar 6, 2023 •
edited
Loading

The line numbers do not match the latest code. I guess it was testing the old code?

Line numbers in TestHSync are off because a01676a changed that file on master in the meantime.

a01676a is HDDS-8029 which is BEFORE this commit 3bbb574

The lines added by a01676a were not present in your PR, as it was based on a prior state of master:

* 4afa68e460 (master) HDDS-7816. Add DataNode list to the SCM WebUI (#4289)
* ...
* 3bbb5742c5 HDDS-8024. When readChunk from a datanode fails, retry other datanodes. (#4336)
* ...
* a01676a9af HDDS-8029. [hsync] Outputstream in encrypted buckets do not return the correct stream capabilities. (#4316)
* ...
| * 6fa8fc1291 (HDDS-8024-repeat) TEST 10x30x TestHSync,ITestOzoneContractCreate
| * 91958b3e0f (HDDS-8024) Fix test failures.
| * 67a8b10fdb HDDS-8024. Intermittent inconsistent read in HSync tests.
|/  
* 84f1523d24 HDDS-7869. Log configuration on component startup. (#4271)

Now that the PR has been merged and became 3bbb574 on master, line numbers are different, due to the presence of a01676a. But that doesn't mean I was testing code without the fix.

Now I have launched a new run with current master: https://github.com/adoroszlai/hadoop-ozone/actions/runs/4347182297

@adoroszlai
Copy link
Contributor

adoroszlai commented Mar 6, 2023

new run with current master: https://github.com/adoroszlai/hadoop-ozone/actions/runs/4347182297

This one failed, too.

Example:

Tests run: 4, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 54.159 s <<< FAILURE! - in org.apache.hadoop.fs.ozone.TestHSync
org.apache.hadoop.fs.ozone.TestHSync.testO3fsHSync  Time elapsed: 3.85 s  <<< ERROR!
java.io.IOException: Inconsistent read for blockID=conID: 1 locID: 111677748019200001 bcsId: 0 length=97 position=64 numBytesToRead=33 numBytesRead=-1
	at org.apache.hadoop.ozone.client.io.KeyInputStream.checkPartBytesRead(KeyInputStream.java:175)
	at org.apache.hadoop.hdds.scm.storage.MultipartInputStream.readWithStrategy(MultipartInputStream.java:97)
	at org.apache.hadoop.hdds.scm.storage.ExtendedInputStream.read(ExtendedInputStream.java:54)
	at org.apache.hadoop.fs.ozone.OzoneFSInputStream.read(OzoneFSInputStream.java:64)
	at java.io.DataInputStream.read(DataInputStream.java:149)
	at org.apache.hadoop.fs.ozone.TestHSync.runTestHSync(TestHSync.java:211)
	at org.apache.hadoop.fs.ozone.TestHSync.runTestHSync(TestHSync.java:164)
	at org.apache.hadoop.fs.ozone.TestHSync.testO3fsHSync(TestHSync.java:132)

@szetszwo
Copy link
Contributor Author

szetszwo commented Mar 6, 2023

@adoroszlai , thanks for the update. Sorry that I may have misunderstood how the test was run.

This one failed, too.

The line number in this stack trace matched the current code. Let me take a look.

@szetszwo
Copy link
Contributor Author

szetszwo commented Mar 7, 2023

@adoroszlai found that getBlock also does not retry other datnaodes. Submitted #4357

@jojochuang jojochuang added the hbase HBase on Ozone support label Jan 23, 2024
@ivandika3 ivandika3 mentioned this pull request Jan 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jojochuang jojochuang jojochuang approved these changes

@adoroszlai adoroszlai Awaiting requested review from adoroszlai

Assignees

No one assigned

Labels

hbase HBase on Ozone support

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@szetszwo @adoroszlai @jojochuang