cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Stefania (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-7807) Push notification when tracing completes for an operation
Date Fri, 27 Mar 2015 02:02:53 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-7807?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14383203#comment-14383203
] 

Stefania commented on CASSANDRA-7807:
-------------------------------------

Thanks for the extra functionality and tests! :)

Here we go:

{{TraceState.java}}:
- Line 41 : unused import 
- Line 147-149 : I would get the minimum version from the event type since we have it there,
and then encapsulate everything in a connection method for better separation of concerns.
{code}
    if (connection != null &&
        connection.getVersion() >= Server.VERSION_4 &&
        ((Server.ConnectionTracker)connection.getTracker()).isRegistered(Event.Type.TRACE_COMPLETE,
connection.channel()))
    {
        connection.send(new Event.TraceComplete(sessionId));
    }
{code}

{{Event.java}}:
- Lines 32/35 : use Server.VERSION_X instead of hard coded numbers?
- Line 55 : why check in {{deserialize()}} but not in {{serialize()}}?
- Line 436 : the same check is in {{deserialize()}} and {{deserializeEvent()}}. I would either
check at the top level ({{serialize()}} and {{deserialize()}}) and make the individual event
methods internal, or leave the other events alone (the v2 events) and only check in {{TraceEvent}}
- Lines 436/444/452 : 
-- Why repeat {{Server.VERSION_4}} instead of the Event type minimum version?

{{SimpleClient.java}}
- Line 216 : {{msg.error}} is a {{TransportException}} which is not a {{RuntimeException}}?
{code}
throw (RuntimeException)((ErrorMessage)msg).error;
{code}

{{TraceCompleteTest.java}}:
- [OPTIONAL] I think now the tests are a bit redundant:
-- we don't need two clients in {{testTraceCompleteVersion3()}} and {{testTraceCompleteWithProbability()}}
-- {{testTraceCompleteNotRegistered()}} doesn't test much that  {{testTraceComplete()}} doesn't
already test

{{MessagePayloadTest.java}}
- It is failing when run from ant (OK from Intellij however):
{code}
ant -Dtest.name=MessagePayloadTest test

[...]

testMessagePayloadVersion3  Error unconfigured table atable

org.apache.cassandra.exceptions.InvalidRequestException: unconfigured table atable
at org.apache.cassandra.transport.messages.ErrorMessage$1.decode(ErrorMessage.java:133)
at org.apache.cassandra.transport.messages.ErrorMessage$1.decode(ErrorMessage.java:44)
at org.apache.cassandra.transport.Message$ProtocolDecoder.decode(Message.java:264)
at org.apache.cassandra.transport.Message$ProtocolDecoder.decode(Message.java:247)
at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:89)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:333)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:319)
at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:333)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:319)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:163)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:333)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:319)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:787)
at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:130)
at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:511)
at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:468)
at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:382)
at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:354)
at io.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:116)
at io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:137)
at java.lang.Thread.run(Thread.java:745)
{code}

Also ant complains about field accessibility, I believe {{resetCqlQueryHandlerField()}} was
intended to be {{@AfterClass}} and have the {{modifiersField}} made accessible first:
{code}
-    @BeforeClass
+    @AfterClass
     public static void resetCqlQueryHandlerField()
     {
         if (cqlQueryHandlerField == null)
@@ -81,6 +82,7 @@ public class MessagePayloadTest extends CQLTester
         try
         {
             Field modifiersField = Field.class.getDeclaredField("modifiers");
+            modifiersField.setAccessible(true);
             modifiersField.setInt(cqlQueryHandlerField, cqlQueryHandlerField.getModifiers()
| Modifier.FINAL);
{code}

The rest is very good!

> Push notification when tracing completes for an operation
> ---------------------------------------------------------
>
>                 Key: CASSANDRA-7807
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-7807
>             Project: Cassandra
>          Issue Type: Sub-task
>          Components: Core
>            Reporter: Tyler Hobbs
>            Assignee: Robert Stupp
>            Priority: Minor
>              Labels: client-impacting, protocolv4
>             Fix For: 3.0
>
>         Attachments: 7807-v2.txt, 7807.txt
>
>
> Tracing is an asynchronous operation, and drivers currently poll to determine when the
trace is complete (in a loop with sleeps).  Instead, the server could push a notification
to the driver when the trace completes.
> I'm guessing that most of the work for this will be around pushing notifications to a
single connection instead of all connections that have registered listeners for a particular
event type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message