[
https://issues.apache.org/jira/browse/CASSANDRA-5286?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13688215#comment-13688215
]
Sylvain Lebresne commented on CASSANDRA-5286:
---------------------------------------------
Remarks on that last version:
* In RangeStreamer, we use to throw a RuntimeException if some ranges hadn't
been fetched correctly. If my reading is correct, with this patch, we throw an
AssertionError (with no particular message) which is less helpful. Maybe the
better option would be to have RangeStreamer.fetch() return the
StreamResultFuture and let the caller rethrow a more meaningful exception on
error.
* In StorageService, when restoring replica count, we used to notify the
endpoint even if the streaming failed. Is there a reason this is not preserved
by the patch?
* In StreamResultFuture, I think relying on a simple number of session might be
fragile. Currently, there is no protection against handleSessionComplete being
called twice for the same session. While I haven't checked carefully if this
can happen, I can easily imagine that in the case of a failure this function
would be called twice in a racing way (maybe a node dies and we get a socket
error but also a signal from the failure detector at the same time). So I think
it could be worth protecting against this. Maybe StreamSession could get a UUID
(that could be useful for debugging anyway), and StreamResultFuture can keep a
set of uncomplete sessions instead of a single AtomicInteger?
* StreamManager can "leak" sessions if the exception thrown is not a
StreamException. We should either log a warning in that case, or change
register to be
{noformat}
public void register(final StreamResultFuture result)
{
result.addListener(new Runnable()
{
public run()
{
currentStreams.remote(result.planId);
}
}, MoreExecutors.sameThreadExecutor());
currentStreams.put(result.planId, result);
}
{noformat}
* Feels weird to create a StreamPlan object on thre receiving end, since the
Plan is mostly a builder to initiate streaming. Besides, on the receiving side
we'll only ever have one StreamSession for the plan I believe. So I'd move the
bulk of StreamPlan.execute to StreamResultFuture and maybe just add a static
StreamSession.startReceivingStream() method that does the equivalent of
StreamPlan.bind+execute.
* StreamPlan: can't we make planId and description final? In particular, I
don't think we have to wait for execute() to generate the planId. As for
description, I was liking it a lot more when this was an enum (if only because
that enum documents in one place the different use of streaming).
* In Outgoing/IncomingMessageHandler.run(), shouldn't we inform the session
that something is wrong on a SocketException?
* Seems like ConnectionHandler drops stream message on the floor without any
kind of logging if we're not connected. Would feel safer to either log a
warning or just throw an exception if that's not supposed to happen anyway.
Similarly, in IncomingMessageHandler, I don't think StreamMessage.deserialize
can return a null value, so we should have an assertion rather than just
silently ignoring what would be a programming error..
* I don't think StreamExecutor has to be a ListeningExecutor. So maybe we can
have a proper streaming stage (so the executor metrics are exposed alongside
other stages).
* Nit: in ConnectionHandler.MessageHandler, terminated could just be a volatile.
* Nit: in OutgoingMessageHandler.run(), the 'else if (terminated())' is not
useful since the while loop already checks it.
* Nit: StreamManagerMBean.getCurrentStreamPlans() is redudant since the planId
is part of the StreamState.
I otherwise agree that JMX notification and streaming different sstable version
could (and really should) be left to followup ticket. I'm also fine pushing
finer grained progress report on reads to later.
> Streaming 2.0
> -------------
>
> Key: CASSANDRA-5286
> URL: https://issues.apache.org/jira/browse/CASSANDRA-5286
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Yuki Morishita
> Labels: streaming
> Fix For: 2.0 beta 1
>
>
> 2.0 is the good time to redesign streaming API including protocol to make
> streaming more performant and reliable.
> Design goals that come up in my mind:
> *Better performance*
> - Protocol optimization
> - Stream multiple files in parallel (CASSANDRA-4663)
> - Persistent connection (CASSANDRA-4660)
> *Better control*
> - Cleaner API for error handling
> - Integrate both IN/OUT streams into one session, so the
> components(bootstrap, move, bulkload, repair...) that use streaming can
> manage them easily.
> *Better reporting*
> - Better logging/tracing
> - More metrics
> - Progress reporting API for external client
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
