You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "maedhroz (via GitHub)" <gi...@apache.org> on 2023/03/23 19:57:40 UTC
[GitHub] [cassandra] maedhroz opened a new pull request, #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
maedhroz opened a new pull request, #2241:
URL: https://github.com/apache/cassandra/pull/2241
patch by Caleb Rackliffe; reviewed by ? for CASSANDRA-18355
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a diff in pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#discussion_r1146777035
##########
src/java/org/apache/cassandra/service/accord/AccordKeyspace.java:
##########
@@ -652,8 +646,9 @@ public static Command loadCommand(AccordCommandStore commandStore, TxnId txnId)
return Command.SerializerSupport.committed(attributes, status, executeAt, promised, accepted, waitingOnCommit, waitingOnApply);
case PreApplied:
case Applied:
+ return Command.SerializerSupport.executed(attributes, status, executeAt, promised, accepted, waitingOnCommit, waitingOnApply, writes, Result.APPLIED);
Review Comment:
Nobody does anything w/ this right now anyway, but I don't know if we need to distinguish between pre-applied and applied...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a diff in pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#discussion_r1149599401
##########
src/java/org/apache/cassandra/service/accord/serializers/ApplySerializers.java:
##########
@@ -52,7 +50,7 @@ public Apply deserializeBody(DataInputPlus in, int version, TxnId txnId, Partial
CommandSerializers.timestamp.deserialize(in, version),
DepsSerializer.partialDeps.deserialize(in, version),
CommandSerializers.writes.deserialize(in, version),
- TxnData.serializer.deserialize(in, version));
+ null);
Review Comment:
so `Apply` only touches this by passing it to `accord.local.SafeCommand#preapplied`, which calls `accord.local.Command.Executed#executed(accord.local.CommonAttributes, accord.local.SaveStatus, accord.primitives.Timestamp, accord.primitives.Ballot, accord.primitives.Ballot, accord.local.Command.WaitingOn, accord.primitives.Writes, accord.api.Result)`... Given the fact that this case maps to `Result.APPLIED` later on I think its fine to drop, internally we can still pass `Result.APPLIED` in
@belliottsmith thoughts?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#issuecomment-1581194712
Note: Locally, `AsyncLoaderTest` and `AsyncOperationTest` seem to pass for me.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a diff in pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#discussion_r1146776160
##########
.gitmodules:
##########
@@ -1,4 +1,4 @@
[submodule "modules/accord"]
path = modules/accord
- url = https://github.com/apache/cassandra-accord.git
- branch = trunk
+ url = ../cassandra-accord.git
+ branch = CASSANDRA-18355
Review Comment:
This obv. reverts before committing.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a diff in pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#discussion_r1154867217
##########
src/java/org/apache/cassandra/service/accord/serializers/ApplySerializers.java:
##########
@@ -52,7 +50,7 @@ public Apply deserializeBody(DataInputPlus in, int version, TxnId txnId, Partial
CommandSerializers.timestamp.deserialize(in, version),
DepsSerializer.partialDeps.deserialize(in, version),
CommandSerializers.writes.deserialize(in, version),
- TxnData.serializer.deserialize(in, version));
+ null);
Review Comment:
```
Caused by: java.lang.IllegalArgumentException: Unexpected state FINISHED
at accord.utils.Invariants.illegalArgument(Invariants.java:54)
at accord.utils.Invariants.checkArgument(Invariants.java:202)
at org.apache.cassandra.service.accord.async.AsyncOperation.fail(AsyncOperation.java:182)
at org.apache.cassandra.service.accord.async.AsyncOperation.run(AsyncOperation.java:273)
at org.apache.cassandra.service.accord.async.AsyncOperation.callback(AsyncOperation.java:157)
at accord.utils.async.AsyncCallbacks.lambda$inExecutor$0(AsyncCallbacks.java:31)
```
that's a known issue
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a diff in pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#discussion_r1154867917
##########
src/java/org/apache/cassandra/service/accord/serializers/ApplySerializers.java:
##########
@@ -52,7 +50,7 @@ public Apply deserializeBody(DataInputPlus in, int version, TxnId txnId, Partial
CommandSerializers.timestamp.deserialize(in, version),
DepsSerializer.partialDeps.deserialize(in, version),
CommandSerializers.writes.deserialize(in, version),
- TxnData.serializer.deserialize(in, version));
+ null);
Review Comment:
[CASSANDRA-18377](https://issues.apache.org/jira/browse/CASSANDRA-18377): CEP-15 (Accord) AsyncOperation can not fail as it has already reached FINISHED
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a diff in pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#discussion_r1146847221
##########
src/java/org/apache/cassandra/service/accord/AccordKeyspace.java:
##########
@@ -652,8 +646,9 @@ public static Command loadCommand(AccordCommandStore commandStore, TxnId txnId)
return Command.SerializerSupport.committed(attributes, status, executeAt, promised, accepted, waitingOnCommit, waitingOnApply);
case PreApplied:
case Applied:
+ return Command.SerializerSupport.executed(attributes, status, executeAt, promised, accepted, waitingOnCommit, waitingOnApply, writes, Result.APPLIED);
Review Comment:
`org.apache.cassandra.service.accord.AccordObjectSizes#command` needs to be updated right?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a diff in pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#discussion_r1151110912
##########
src/java/org/apache/cassandra/service/accord/serializers/ApplySerializers.java:
##########
@@ -52,7 +50,7 @@ public Apply deserializeBody(DataInputPlus in, int version, TxnId txnId, Partial
CommandSerializers.timestamp.deserialize(in, version),
DepsSerializer.partialDeps.deserialize(in, version),
CommandSerializers.writes.deserialize(in, version),
- TxnData.serializer.deserialize(in, version));
+ null);
Review Comment:
After making that change, I'm getting another simulator failure: https://app.circleci.com/pipelines/github/maedhroz/cassandra/709/workflows/221592d1-1ba2-4df3-a22a-c51ae76edd08/jobs/7699/tests#failed-test-0
Will see if I can reproduce locally here...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#issuecomment-1481814022
https://app.circleci.com/pipelines/github/maedhroz/cassandra?branch=CASSANDRA-18355
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz closed pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
URL: https://github.com/apache/cassandra/pull/2241
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#issuecomment-1753239492
Committed as https://github.com/apache/cassandra/commit/56e218638db6816f9a6fa66fa2b6fc35977a92a2
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a diff in pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#discussion_r1146848482
##########
src/java/org/apache/cassandra/service/accord/AccordKeyspace.java:
##########
@@ -652,8 +646,9 @@ public static Command loadCommand(AccordCommandStore commandStore, TxnId txnId)
return Command.SerializerSupport.committed(attributes, status, executeAt, promised, accepted, waitingOnCommit, waitingOnApply);
case PreApplied:
case Applied:
+ return Command.SerializerSupport.executed(attributes, status, executeAt, promised, accepted, waitingOnCommit, waitingOnApply, writes, Result.APPLIED);
Review Comment:
think serializing `accord.messages.CheckStatus.CheckStatusOkFull#result` also needs to be updated? Which should be `org.apache.cassandra.io.IVersionedSerializer#serialize`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a diff in pull request #2241: CASSANDRA-18355 Avoid serializing full TxnData instances to Accord state tables
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2241:
URL: https://github.com/apache/cassandra/pull/2241#discussion_r1146994658
##########
src/java/org/apache/cassandra/service/accord/serializers/ApplySerializers.java:
##########
@@ -52,7 +50,7 @@ public Apply deserializeBody(DataInputPlus in, int version, TxnId txnId, Partial
CommandSerializers.timestamp.deserialize(in, version),
DepsSerializer.partialDeps.deserialize(in, version),
CommandSerializers.writes.deserialize(in, version),
- TxnData.serializer.deserialize(in, version));
+ null);
Review Comment:
Wondering if we should just remove the `Result` argument from `create()` entirely...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org