[
https://issues.apache.org/jira/browse/CASSANDRA-14781?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17015369#comment-17015369
]
Venkata Harikrishna Nukala commented on CASSANDRA-14781:
--------------------------------------------------------
[~jrwest]
{quote}A few code review comments below. I did want to discuss if we are going
to address the user facing concerns Aleksey brought up in this ticket? The
patch addresses the operators lack of visibility into keyspace/table/partitions
but still results in timeouts for the user. Are we going to address those in a
separate ticket? My thought is that something for the operators is better than
no patch (having been blind in this situation before besides custom tools) but
if the user facing changes require protocol changes we should probably fix it
pre-4.0 like we have or plan to w/ other similar tickets – but that could still
be in a separate ticket.
{quote}
I would prefer to have a separate ticket. +1 on having something better than no
patch.
{quote}
* We also shouldn’t duplicate the implementations between counter and regular
mutations
* validateSize: since the two implementations are identical you could move them
to a default implementation in IMutation
{quote}
validateSize methods implementation looks similar, but they use different
serialisers (Mutation uses MutationSerializer and CounterMutation uses
CounterMutationSerializer) which are not visible in IMutation interface.
serializedSize() methods needs memoization (needs serializedSize* fields) be
cause of which we cannot move to interface as fields will be final. We had
ruled out option having a separate class (i.e. SizeHolder). Now it makes me
think about 2 options.
1. I could not find any validation of size for {{CounterMutation}}. If it is
expected or not required, then can remove {{CounterMutaiton}} changes and
use\{{ Mutation.validateSize}} directly (instead of defining it in
{{IMutation}}). The disadvantage I see with this approach is caller has to be
aware of implementation and it makes things hard to abstract (code has to be
aware of implementation instead of {{IMutation}}).
2. Expect {{VirtualMutaiton}}, I see mutations are expected to be serialized
and/or deserialized. Provide serialize, serialziedSize and deserialize methods
as part of {{IMutaiton}} (so that we can abstract out direct usages of
{{Mutation.serializer}} and {{CounterMutation.serializer}}) with an abstract
class in between having common functionality.
Or else pay the price of duplicate code. What do you think?
{quote}MaxMutationExceededException: the sort in #prepareMessage could get
pretty expensive, is it necessary?
{quote}
In Mutation, I see that there is only one PartitionUpdate per Table, and
according to Mutation.merge() logic, a mutation can have changes related to
only one keyspace and one key. Even if there are multiple updates for different
rows of same partition, they are merged into single PartitionUpdate.
When I ran a small test for sorting list of Longs (laptop having i7, 6 core and
16gb ram) it took approximately 33ms, 6ms and 1ms for 100K, 10k and 1k
respectively.
According to merge logic and test numbers, unless there are thousands of tables
in a key space and trying to update all of them at once, I dont see a scenario
where sorting can hurt (time taken for sort > 1 or 2ms).
{quote}It also looks like there is an edge case where “and more” will be added
even when there aren’t more. Using listIterator.hasNext() instead of
topPartitions.size() > 0 should fix that
{quote}
I had moved the code into separte funtion and added unit test cases. It is
working as expected. Using listIterator.hasNext() caused few tests to fail. Did
I miss any scenario to test?
Converted serializedSize* long fields to int as suggested by Aleksey. Changes
are here:
https://github.com/apache/cassandra/compare/trunk...nvharikrishna:14781-trunk?expand=1
> Log message when mutation passed to CommitLog#add(Mutation) is too large is
> not descriptive enough
> --------------------------------------------------------------------------------------------------
>
> Key: CASSANDRA-14781
> URL: https://issues.apache.org/jira/browse/CASSANDRA-14781
> Project: Cassandra
> Issue Type: Bug
> Components: Consistency/Hints, Local/Commit Log, Messaging/Client
> Reporter: Jordan West
> Assignee: Tom Petracca
> Priority: Normal
> Labels: protocolv5
> Fix For: 4.0-beta
>
> Attachments: CASSANDRA-14781.patch, CASSANDRA-14781_3.0.patch,
> CASSANDRA-14781_3.11.patch
>
>
> When hitting
> [https://github.com/apache/cassandra/blob/cassandra-3.0/src/java/org/apache/cassandra/db/commitlog/CommitLog.java#L256-L257],
> the log message produced does not help the operator track down what data is
> being written. At a minimum the keyspace and cfIds involved would be useful
> (and are available) – more detail might not be reasonable to include.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
