[
https://issues.apache.org/jira/browse/CASSANDRA-14781?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jordan West updated CASSANDRA-14781:
------------------------------------
Reviewers: Chris Lohfink, Jordan West, Jordan West (was: Chris Lohfink,
Jordan West)
Chris Lohfink, Jordan West, Jordan West (was: Chris Lohfink)
Status: Review In Progress (was: Patch Available)
Thanks for picking this up [~n.v.harikrishna]. Comments below.
* General:
* Since this change logs keys which in some use cases may contain PII that
users may not want logged, should we add a flag to gate this feature?
* I don’t believe this address [~aleksey]'s concern that the exception be
propagated to the client vs. timing out. At a minimum, we should add an in-jvm
dtest to show it does (I’m happy to help contribute this unless you’d like to).
Its possible to split the patch into two parts (one that changes the logging
and one that propagates the exception), however, in prod I’ve seen the lack of
this logging and the timeouts be an issue so they are definitely both worth
addressing.
* CommitLog:
* With your change to validate the size using serializedSize, it looks like
its no longer be necessary to use the scratch buffer (which seems to be used
primarily to get the length w/o calculating serialized size). However, I wonder
if the original approach is better performance wise since we are already
serializing the mutation. It would be good to benchmark workloads with all
mutations exceeding max size, no mutations exceeding max size, and mixed to
check because I can imagine how what I said is wrong as well. We should check
if there is any difference or if its negligible.
* SizeHolder:
* Since the only call to \{{#getSize()}} currently is
\{{IMutation#validateSize}} which is only called once from CommitLog.java.
Consider removing the memoization.
* If the memoization is still desired, I don’t think its necessary to have
fields for each version since a mutation is executed within a single version of
Cassandra (it may cross network boundaries and thus versions but at this point
we have a new SizeHolder). Using a single variable would cleanup \{{#getSize}}
considerably. Further, it would be cleaner/less error prone to memoize the size
in the mutation since nothing maps the object passed to \{{#getSize}} to the
stored value.
* MutationExceededMaxSizeException:
* {\{prepareMessage}} recalculates the mutation size. While this is an
exceptional case it is still common. Perhaps a place where the memoization
could be helpful?
* Spelling error: maxMutaionSize
* I almost always prefer a configurable limit to something hardcoded and
requiring recompilation to change
* I don’t think \{{IMutation.getMaxMutationSize()}} is necessary. Use
\{{DatabaseDescriptor}} instead. This is more typical of Cassandra config code.
Further, the original code only called
\{{DatabaseDescriptor.getMaxMutationSize()}} once instead of for every write.
Unless we are going to make it a hot-prop its worth putting back to how it was.
* We have a few \{{Serializer}} interfaces in C* already, consider renaming
\{{SizeHolder.Serializer}} if its kept
> 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
