[
https://issues.apache.org/jira/browse/HDDS-1348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16813680#comment-16813680
]
Shashikant Banerjee edited comment on HDDS-1348 at 4/9/19 6:08 PM:
-------------------------------------------------------------------
Thanks [~jnp] for the review.
{noformat}
Why don't we always call adjustBuffers on
xceiverClient.getReplicatedMinCommitIndex()? Isn't replicated-min-commit-index
is the source of truth in all cases.{noformat}
In case, watchForCommit is successful, it returns the latest successfully
replicated log index. There is no need of calling minReplicatedLogIndex again
while discarding the buffer. In case of an exception, we need to know what was
the last successfully replicated log index and discard buffers accordingly.
{code:java}
I think totalAckDataLength should be updated as soon as we get an ack, instead
of adjustBuffer. We are coupling the buffer-management with tracking of acks.
{code}
Total ack length is updated only by the position of the buffers being discarded
at the time when watchForCommit ends up discarding buffers and hence the length
gets updated there as that is where we ensure how much of data is ack'd by
servers. We cannot update the ack length as soon as the putBlock ack comes, as
it does not guarantee data being successfully replicated to all servers.
{code:java}
watchOnCommitIndex(boolean first): It would be better to have two different
functions, one for buffer full case, and another for flush. A boolean flag can
be confusing for example, suppose when we call flush, buffer is full as well,
so it becomes tricky to understand code
{code}
Addressed in patch v2.
{noformat}
Please add unit tests only for CommitWatcher class. The test should mock
xceiverClient, and test buffers are appropriately released, for exception as
well as non-exception cases.{noformat}
Addressed in patch v2.
{code:java}
Let's make CommitWatcher#getCommitIndex2flushedDataMap visible for testing.
{code}
Already exposed for testing by
BlockOutputStream#getCommitWatcher#getCommitIndex2flushedDataMap() as well as
verified in TestBlockOutputStream and TestBlockOutputStreamFailures.
was (Author: shashikant):
Thanks [~jnp] for the review.
{noformat}
Why don't we always call adjustBuffers on
xceiverClient.getReplicatedMinCommitIndex()? Isn't replicated-min-commit-index
is the source of truth in all cases.{noformat}
In case, watchForCommit is successful, it returns the latest successfully
replicated log index. There is no need of calling minReplicatedLogIndex again
while discarding the buffer. In case of an exception, we need to know what was
the last successfully replicated log index and discard buffers accordingly.
{code:java}
I think totalAckDataLength should be updated as soon as we get an ack, instead
of adjustBuffer. We are coupling the buffer-management with tracking of acks.
{code}
Total ack length is updated only by the position of the buffers being discarded
at the time when watchForCommit ends up discarding buffers and hence the length
gets updated there as that is where we ensure how much of data is ack'd by
servers. We cannot update the ack length as soon as the putBlock ack comes, as
it does not guarantee data being successfully replicated to all servers.
{code:java}
watchOnCommitIndex(boolean first): It would be better to have two different
functions, one for buffer full case, and another for flush. A boolean flag can
be confusing for example, suppose when we call flush, buffer is full as well,
so it becomes tricky to understand code
{code}
Addressed in patch v2.
{noformat}
Please add unit tests only for CommitWatcher class. The test should mock
xceiverClient, and test buffers are appropriately released, for exception as
well as non-exception cases.{noformat}
Addressed in patch v2.
{code:java}
Let's make CommitWatcher#getCommitIndex2flushedDataMap visible for testing.
{code}
Already exposed for testing by
BlockOutputStream#getCommitWatcher#getCommitIndex2flushedDataMap() as well as
verified in TestBlockOutputStream and TestBlockOutputStreamFailures.
> Refactor BlockOutpuStream Class
> -------------------------------
>
> Key: HDDS-1348
> URL: https://issues.apache.org/jira/browse/HDDS-1348
> Project: Hadoop Distributed Data Store
> Issue Type: Improvement
> Components: Ozone Client
> Reporter: Shashikant Banerjee
> Assignee: Shashikant Banerjee
> Priority: Major
> Fix For: 0.5.0
>
> Attachments: HDDS-1348.000.patch, HDDS-1348.001.patch
>
>
> BlockOutputStream contains functionalities for handling write, flush and
> close as well as tracking commitIndexes . The idea is to separate all
> commitIndex tracking and management code outside of BlockOutputStream class
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
