Skip to content

Conversation

@captainzmc
Copy link
Member

What changes were proposed in this pull request?

See: https://issues.apache.org/jira/browse/HDDS-6130

How was this patch tested?

Existing tests: testPutBlockAtBoundary

Copy link
Contributor

@sadanand48 sadanand48 left a comment

Choose a reason for hiding this comment

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

Thanks @captainzmc for the PR. I just realised that this problem is due to the wrongly computed totalAckDataLength. Since we are not doing watchForCommit() after periodic putBlock() i.e in doFlushIfNeeded() , it might happen that the same set of buffers are inserted into the commitInfoMap more than once because of which totalAckData is wrongly computed and we get this error. I think for now we can just add watchForCommit() after doing putBlock in here which should solve the issue right away,

@captainzmc
Copy link
Member Author

captainzmc commented Dec 22, 2021

I think for now we can just add watchForCommit() after doing putBlock in here which should solve the issue right away,

Thanks @sadanand48 for your advice, I have updated the PR and the testPutBlockAtBoundary can be used to verify this fix.

Through testing, I found that only add watchForCommit() is not enough. ExecutePutBlock() is only an asynchronous to execute updateCommitInfoMap. we should add waitOnFlushFutures() for ensure that updateCommitInfoMap is executed. Without waitOnFlushFutures() we'll have the same problem.

@captainzmc
Copy link
Member Author

captainzmc commented Dec 22, 2021

@sadanand48
Oops! I found that our cleanup of futureMap in releaseBuffers was redundant. We have called the commitWatcher cleanup in close. Since futureMap doesn't store much and doesn't affect the client's memory, we can simply clean up it on close.
Let's just delete this part so it looks simpler and cleaner.

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@captainzmc , you are right that the error messages are inaccurate.

After the change, the futureMap is no longer useful. Let's remove it; see https://issues.apache.org/jira/secure/attachment/13037847/2939_review.patch

@captainzmc captainzmc force-pushed the HDDS-4454-1221-2 branch 3 times, most recently from 2e0db27 to 06a2e58 Compare December 23, 2021 14:25
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

+1 the change looks good.

@szetszwo szetszwo merged this pull request into apache:HDDS-4454 Dec 23, 2021
szetszwo pushed a commit that referenced this pull request Dec 30, 2021
szetszwo pushed a commit that referenced this pull request Jan 20, 2022
captainzmc added a commit that referenced this pull request Feb 9, 2022
szetszwo pushed a commit that referenced this pull request Feb 16, 2022
szetszwo pushed a commit that referenced this pull request Mar 15, 2022
szetszwo pushed a commit that referenced this pull request Mar 24, 2022
szetszwo pushed a commit to szetszwo/ozone that referenced this pull request May 6, 2022
szetszwo pushed a commit that referenced this pull request May 13, 2022
szetszwo pushed a commit that referenced this pull request May 24, 2022
szetszwo pushed a commit that referenced this pull request Jun 9, 2022
captainzmc added a commit to captainzmc/hadoop-ozone that referenced this pull request Jul 4, 2022
szetszwo pushed a commit that referenced this pull request Oct 25, 2022
…find the required future” (#2939)

(cherry picked from commit 2aacc7a)
(cherry picked from commit 1ecb774a39971f264325b6ce2ab0d4cbe4025ee6)
szetszwo pushed a commit that referenced this pull request Nov 7, 2022
…find the required future” (#2939)

(cherry picked from commit 2aacc7a)
(cherry picked from commit 1ecb774a39971f264325b6ce2ab0d4cbe4025ee6)
(cherry picked from commit ac981a7)
szetszwo pushed a commit that referenced this pull request Dec 1, 2022
…find the required future” (#2939)

(cherry picked from commit 58da69f)
szetszwo pushed a commit that referenced this pull request Dec 16, 2022
…find the required future” (#2939)

(cherry picked from commit 58da69f)
nishitpatira pushed a commit to nishitpatira/ozone that referenced this pull request Dec 16, 2022
…find the required future” (apache#2939)

(cherry picked from commit 58da69f)
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.

3 participants