Consolidate batch write code

[ifesdjeen]

for CASSANDRA-14742

[belliottsmith]

Patch looks good overall, just have some super minor nits.

[belliottsmith]

indentation?

[belliottsmith]

should EndpointFilter be brought into ReplicaPlans? It's not used anywhere else, and it seems that the two pieces of logic should be proximal

[ifesdjeen]

I wanted to, but it's also fairly large, so wanted to avoid polluting it. If you think it's a good idea, i can definitely do it.

[belliottsmith]

I don't love it either, but it should live together. Perhaps we can clean it up, and we can certainly at least make it a static method instead of a class.

[ifesdjeen]

Moved it

[belliottsmith]

If we static import the inner class, we can avoid the unnecessary newline (which irks me unreasonably, because it's anyway going over the line length limit)

[ifesdjeen]

Fixed

[belliottsmith]

I thought we had ended up calling this isSelf because isLocal is overloaded in this code (meaning, variably, is this instance (has our broadcast address); is the same DC as this instance; has the 'local' address). Not strictly related to this patch, but perhaps we should rename to isSelf while we're here and remember? This is consistent with the nomenclature amongst the TR patch, and in ReplicaCollection (withoutSelf and selfIfPresent)

[ifesdjeen]

Renamed

[belliottsmith]

Do we need this check, if we have assureSufficientReplicas below?

[ifesdjeen]

We don't strictly need it: included it only to short-circuit. We can skip it.

[belliottsmith]

Are we sure it's OK (although consistent with prior behaviour), to only require CL.ONE if all nodes besides us are down? The logic above suggests we only require CL.ONE if we're a single-node DC, but we could have multiple failing nodes in our DC, and be the only one left. It seems like we should probably claim insufficient consistency in this case, though this question probably deserves a separate ticket for discussion.

[ifesdjeen]

Right, I had similar thoughts when writing that. I've also been thinking if it's enough to only have two nodes in local DC blocking for batchlog writes. I wanted to make a change to original CL, but that'd might reduce availability (for instance when it's a quorum). But from looking at batch replay, we should be able to do it.

[belliottsmith]

Hmm. It looks like consistencyLevel is almost completely ignored, besides for ==CL.ANY, but callers seem to then use the provided consistencyLevel to construct an AbstractWriteResponseHandler.

Should we really be accepting consistencyLevel here? All we really need to know is if the caller needs durability (i.e. if CL.ANY was provided), and perhaps we should have a separate enum (or just a boolean) for this, as it is otherwise rather confusing. Though what CL.ANY even means for batch log, I haven't a clue, anyway.

Either way, while we're here we should document in the caller the fact that the provided consistencyLevel is not used, and that the batchlogConsistencyLevel is used to clear the entries from the remote batch logs once the real write has reached that consistency level.

[ifesdjeen]

Either way, while we're here we should document in the caller the fact that the provided consistencyLevel is not used, and that the batchlogConsistencyLevel is used to clear the entries from the remote batch logs once the real write has reached that consistency level.

not sure I understand what you mean, as we're using it both for cleanup and for writing to batchlog.

[ifesdjeen]

@belliottsmith I've addressed your comments, rebased and pushed for one more round of CI.