STAR-1904: Take into account primary key size for batch size guardrail

[szymon-miezal]

The size of a mutation does not consider the primary key size. In the context of BATCHed mutations, this means that INSERTs, DELETEs, and UPDATEs for tables with a simple PRIMARY KEY and no clustering columns would be equal to zero. Consequently, the batch_size_fail_threshold_in_kb has no effect for such tables, and it cannot protect the cluster from being overloaded.

This patch modifies the batch size calculation formula to consider the primary key size.

The issue was originally fixed in DSE 5.1/6.8 under https://datastax.jira.com/browse/DSP-23913.

[szymon-miezal]

Tests are in progress: https://jenkins-stargazer.aws.dsinternal.org/job/ds-cassandra-pr-gate/view/change-requests/job/PR-958/2/.

[szymon-miezal]

It seems butler is finally happy https://github.com/datastax/cassandra/commit/553fbb36fcfeb9e440a668bbcac4577baad80a6b#commitcomment-138115776

[JeremiahDJordan]

Thinking about this some more, I am worried some about this immediately breaking a bunch of users workloads without any warning. I wonder if we should track the primary key size in a separate counter and then based on a -D decide to use it for the failure decision. If it is not used in the failure calculation, but it would put over threshold, then a warning could be emitted. That way anyone who has tuned their workload to be just under the threshold using the current calculation would not start failing immediately on upgrade... But could see the WARN and either lower their batch size or raise their threshold based on the new calculation.

[szymon-miezal]

Thank you that's a good point and yes, that's the disadvantage of this patch, it lacks backward compatibility. Workloads that executed fine without warning or errors now might be emitting warnings or even be rejected at all. As you suggest we could add a flag that by default would be false meaning that in case the new formula yields value over the error threshold it will gracefully tell us about it in an additional log message. Changing the flag value to true would be equivalent to the current behaviour of this patch. The downside of this approach would be that:

• The guardrail will not work correctly by default.
• We introduce a flag that's going to be difficult to remove in the future. Alternatively, we might consider documenting (in release notes or upgrade guidelines) that if it's a non-backward compatible change the value of the batch guardrail should be adjusted accordingly.

[JeremiahDJordan]

Alternatively, we might consider documenting (in release notes or upgrade guidelines) that if it's a non-backward compatible change the value of the batch guardrail should be adjusted accordingly.

Nobody knows how big to set it to. So release notes don’t really help. We should never make a change in a patch release that can knowingly break an existing working workload.

another option would be to make a brand new guardrail “batch size with key” or something which defaults to -1 and then someone who cares can turn that on knowing they may need to tune their apps after doing so.