dcapwell commented on PR #3486: URL: https://github.com/apache/cassandra/pull/3486#issuecomment-2313302319
fixed all feedback or asked for more details (also poked you in slack) you have 2 similar / related feedbacks, so trying to address here (GH isn't letting me reply...) https://github.com/apache/cassandra/pull/3486#pullrequestreview-2263041945 - I tested out a `CommandsBuilder` and it made the test `RouteIndextTest` in cep-15-accord easier to read, but I don't see how it helps in this case due to 2 variables that I am not sure how to handle (without making the builder specific to this class) ``` final List<Runnable> preActions = new CopyOnWriteArrayList<>(); final List<Runnable> onError = new CopyOnWriteArrayList<>(); ``` The `State` has a `preActions` and `onError` actions... these kinda distort the model and are plugged into the `Commands` functions... if I move to a `CommandsBuilder` model I think `onError` doesn't really change much, but `preActions` gets funky. Personally I feel the `preActions` is a hack... it would be great to return `Command` so the history does reflect those steps... but that would kinda require each `Command` returned to be a `MultiCommand`... and what about actions that don't mutate anything and just update the `State`'s bookkeeping? those don't change the history and would just clutter up the error message... https://github.com/apache/cassandra/pull/3486#discussion_r1732701880 I don't see how a builder works in this specific case. We are creating a single `Command` and this command has 3 behaviors: if topology changed do read, 20% of the time do reads, 80% of the time do writes.... the builder would need to handle the `if topology changed` bit... which seems funky... and doing a builder for the 20%/80% case seems more verbose than the current logic -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
