[ https://issues.apache.org/jira/browse/CASSANDRA-14781?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17012980#comment-17012980 ]
Jordan West edited comment on CASSANDRA-14781 at 1/10/20 4:24 PM: ------------------------------------------------------------------ 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. Code Comments: * -{{Mutation#serializedSize}}: you should only need one field to memoize the size and can you pass version directly to {{Serializer#serializedSize}} instead of the switch afterwards?- never mind [~n.v.harikrishna] pointed out code in {{BlockingReadRepairs}} that makes this statement false. * 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}} * {\{MaxMutationExceededException}}: the sort in {{#prepareMessage}} could get pretty expensive, is it necessary? 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 was (Author: jrwest): 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. Code Comments: * {{Mutation#serializedSize}}: you should only need one field to memoize the size and can you pass version directly to {{Serializer#serializedSize}} instead of the switch afterwards? * 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}} * {\{MaxMutationExceededException}}: the sort in {{#prepareMessage}} could get pretty expensive, is it necessary? 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 > 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