-
Notifications
You must be signed in to change notification settings - Fork 592
HDDS-5961. [Ozone-Streaming] update the usage space of Containers in … #2833
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.
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
HDDS-5961. [Ozone-Streaming] update the usage space of Containers in … #2833
Conversation
|
hi @captainzmc , can you help review this patch? |
da03fff to
316ef1f
Compare
|
The old StreamDataChannel implementation did not update the usedBytes in the Container metadata at write time. |
|
hi @szetszwo , can you help review this patch? |
|
@guohao-rosicky , I just have sync'ed the HDDS-4454 branch with master. Could you update this pull request? |
316ef1f to
3b3fb1a
Compare
|
@szetszwo @captainzmc , I have completed the update. |
3b3fb1a to
7df8c76
Compare
captainzmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, Failed UT may not be caused by this pr, it can be triggered again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we had updae metrics.incContainerBytesStats and containerData.updateWriteStats. Can you also add some test case? Just make sure that future changes do not affect this logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captainzmc ,Has been modified.
szetszwo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guohao-rosicky , thanks a lot for working on this. Some comments inlined; see also https://issues.apache.org/jira/secure/attachment/13036136/2833_review.patch .
BTW, we use 4 spaces for continuation indent in Ozone. Please change the continuation indent to 4 spaces (instead of 8). Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a new Type, say StreamWrite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szetszwo is container type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove LOG since it is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ContainerData.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ContainerData so that we do not need to cast it when calling the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @Override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add default to throw UnsupportedOperationException so that the tests won't have to override it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not try-catch and throws StorageContainerException in the getStreamDataChannel(..) declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @Override.
7df8c76 to
b2f4cce
Compare
|
Has been modified, @szetszwo @captainzmc .Please Take A Look. |
szetszwo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 the latest change looks good.
…the stream write (apache#2833)
…the stream write (apache#2833)
…the stream write (apache#2833) (cherry picked from commit 7290ff9)
HDDS-5961. [Ozone-Streaming] update the usage space of Containers in the stream write
jira: https://issues.apache.org/jira/browse/HDDS-5961