[jira] [Commented] (OAK-4803) Simplify the client side of the cold standby
[
https://issues.apache.org/jira/browse/OAK-4803?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15493120#comment-15493120
]
Andrei Dulceanu commented on OAK-4803:
--
[~frm], I guess you're right with the separation of concerns pointed out for my
1st suggestion.
bq. Is this maybe the scope of another improvement issue?
Maybe it makes sense to move this to another issue, since this issue already
handles quite a number of aspects.
bq. I quite don't agree here. You need a client to perform a sync.
Reading the whole explanation, I think it's a valid point of view so I will
adhere to it :)
> Simplify the client side of the cold standby
>
>
> Key: OAK-4803
> URL: https://issues.apache.org/jira/browse/OAK-4803
> Project: Jackrabbit Oak
> Issue Type: Improvement
> Components: segment-tar
>Reporter: Francesco Mari
>Assignee: Francesco Mari
> Fix For: Segment Tar 0.0.12
>
> Attachments: OAK-4803-01.patch
>
>
> The implementation of the cold standby client is overly and unnecessarily
> complicated. It would be way clearer to separate the client code in two major
> components: a simple client responsible for sending messages to and receive
> responses from the standby server, and the synchronization algorithm used to
> read data from the server and to save read data in the local {{FileStore}}.
> Moreover, the client simple client could be further modularised by
> encapsulating request encoding, response decoding and message handling into
> their own Netty handlers.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (OAK-4803) Simplify the client side of the cold standby
[
https://issues.apache.org/jira/browse/OAK-4803?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15493103#comment-15493103
]
Francesco Mari commented on OAK-4803:
-
bq. When looking at StandbyClient and StandbyServer I find it a little bit odd
that the former doesn't have a FileStore, while the latter has. IMHO having a
FileStore reference in the client would make sense and would also help reading
the code, as it is much clearer from the beginning who owns what.
This comment makes very much sense, because it highlights an asymmetry between
the client and the server. To be honest, I prefer the way the client is
written, without an explicit reference to the {{FileStore}}. This highlights
the fact that the client is concerned with sending requests and parsing
responses from the server, instead of taking care of how the data is actually
used. I could have achieved the same separation of concerns in the server as
well by introducing a very small interface. In the end, these are the only
three lines where the {{FileStore}} is used in the server, which already
suggests that this separation of concerns exists - at least at the level of the
handlers.
{noformat}
p.addLast(new GetHeadRequestHandler(new DefaultStandbyHeadReader(store)));
p.addLast(new GetSegmentRequestHandler(new DefaultStandbySegmentReader(store)));
p.addLast(new GetBlobRequestHandler(new DefaultStandbyBlobReader(store)));
{noformat}
Is this maybe the scope of another improvement issue?
bq. It looks more natural to have a client which wants to perform a sync as
opposed to have a sync which will create a client.
I quite don't agree here. You need a client to perform a sync. The client could
be used for different purposes other than the sync, so it makes sense of having
the sync process depending on a client and not the other way around. Anyway,
the lines you pointed out are leftovers from the refactoring and I recognise
that they are confusing. I will clean them up.
bq. One minor change in {{StandbySync.run()}}, to allow the state to actually
enter {{STATUS_STARTING}}.
Good catch. It has to be cleaned up as part of this patch.
bq. Rename {{copySegmentFromPrimary}} to {{copySegmentHierarchyFromPrimary}} or
any other explanatory method name, since this method does a BFS starting with
the initial segment to fetch from server.
Nice suggestion. That method should have a more appropriate name, and I like
your proposal.
> Simplify the client side of the cold standby
>
>
> Key: OAK-4803
> URL: https://issues.apache.org/jira/browse/OAK-4803
> Project: Jackrabbit Oak
> Issue Type: Improvement
> Components: segment-tar
>Reporter: Francesco Mari
>Assignee: Francesco Mari
> Fix For: Segment Tar 0.0.12
>
> Attachments: OAK-4803-01.patch
>
>
> The implementation of the cold standby client is overly and unnecessarily
> complicated. It would be way clearer to separate the client code in two major
> components: a simple client responsible for sending messages to and receive
> responses from the standby server, and the synchronization algorithm used to
> read data from the server and to save read data in the local {{FileStore}}.
> Moreover, the client simple client could be further modularised by
> encapsulating request encoding, response decoding and message handling into
> their own Netty handlers.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (OAK-4803) Simplify the client side of the cold standby
[
https://issues.apache.org/jira/browse/OAK-4803?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15492953#comment-15492953
]
Andrei Dulceanu commented on OAK-4803:
--
@frm, here are some of my observations regarding the patch:
# When looking at {{StandbyClient}} and {{StandbyServer}} I find it a little
bit odd that the former doesn't have a {{FileStore}}, while the latter has.
IMHO having a {{FileStore}} reference in the client would make sense and would
also help reading the code, as it is much clearer from the beginning who owns
what.
# Along the same lines, IMHO it would make sense to reverse the relationship
between {{StandbyClient}} and {{StandbySync}} since it looks more natural to
have a client which wants to perform a sync as opposed to have a sync which
will create a client, assign it a file store and then execute the sync. For
example, replacing the line
{code:java}
StandbyClient cl = newStandbyClient(secondary);
{code}
with this line
{code:java}
StandbySync cl = newStandbyClient(secondary);
{code}
seems a little bit confusing to me.
# One minor change in {{StandbySync.run()}}, to allow the state to actually
enter {{STATUS_STARTING}}:
{code:java}
state = STATUS_STARTING;
synchronized (sync) {
if (active) {
return;
}
state = STATUS_RUNNING;
active = true;
}
{code}
# another minor change in {{StandbySyncExecution}}: rename
{{copySegmentFromPrimary}} to {{copySegmentHierarchyFromPrimary}} or any other
explanatory method name, since this method does a BFS starting with the initial
segment to fetch from server.
/cc [~marett]
> Simplify the client side of the cold standby
>
>
> Key: OAK-4803
> URL: https://issues.apache.org/jira/browse/OAK-4803
> Project: Jackrabbit Oak
> Issue Type: Improvement
> Components: segment-tar
>Reporter: Francesco Mari
>Assignee: Francesco Mari
> Fix For: Segment Tar 0.0.12
>
> Attachments: OAK-4803-01.patch
>
>
> The implementation of the cold standby client is overly and unnecessarily
> complicated. It would be way clearer to separate the client code in two major
> components: a simple client responsible for sending messages to and receive
> responses from the standby server, and the synchronization algorithm used to
> read data from the server and to save read data in the local {{FileStore}}.
> Moreover, the client simple client could be further modularised by
> encapsulating request encoding, response decoding and message handling into
> their own Netty handlers.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
