You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/03/01 19:26:42 UTC
[GitHub] [cassandra-accord] bdeggleston opened a new pull request #2: Initial Cassandra integration
bdeggleston opened a new pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2
I'll rebase onto trunk once https://github.com/apache/cassandra-accord/pull/1 is merged
--
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-accord] belliottsmith commented on pull request #2: Initial Cassandra integration
Posted by GitBox <gi...@apache.org>.
belliottsmith commented on pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2#issuecomment-1070892131
I have some suggested improvements to how we manage command stores: https://github.com/belliottsmith/accord/tree/topology-integration-suggest
Also, I think this patch continues to have some issues passing the burn test
--
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-accord] belliottsmith commented on pull request #2: Initial Cassandra integration
Posted by GitBox <gi...@apache.org>.
belliottsmith commented on pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2#issuecomment-1083656938
I think it would be great to pause and let review and feedback catch up, even if there may be some duplicated work or rebasing involved. We haven't yet addressed all feedback from the last PR, and to daisy-chain to a third PR would be less than ideal IMO.
Regarding cassandra-all, there are already C* concepts that I think we would benefit from having access to, but sure I can bring it in when I need it.
--
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-accord] belliottsmith commented on a change in pull request #2: Initial Cassandra integration
Posted by GitBox <gi...@apache.org>.
belliottsmith commented on a change in pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2#discussion_r825732648
##########
File path: accord-core/src/main/java/accord/messages/ReplyContext.java
##########
@@ -0,0 +1,5 @@
+package accord.messages;
+
+public interface ReplyContext
Review comment:
Could you link to the C* implementation part this maps to? I had sort of imagined it would be a `long` on the C* side as well, given the `RequestCallbacks` uses a long for `messageId`
--
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-accord] bdeggleston commented on pull request #2: Initial Cassandra integration
Posted by GitBox <gi...@apache.org>.
bdeggleston commented on pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2#issuecomment-1086251987
Ok pulled in the suggestions and addressed the missed feedback. For the suggestions, I reverted how command stores detect reintroduced ranges, as the changes would not have detected them properly. Also changed some minor stuff for clarity
--
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-accord] bdeggleston commented on pull request #2: Initial Cassandra integration
Posted by GitBox <gi...@apache.org>.
bdeggleston commented on pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2#issuecomment-1083622913
I reworked this a bit to prevent assigning ranges to nodes that were previously replicas of said ranges. Supporting that opens a can of worms where you have to purge stale data which other nodes might still need. It's probably tractable today, but it will be much easier once repair is merged and we can require purgeable ranges are fully replicated. I also added a throttle on topology changes since the topology randomizer was keeping some burn tests busy indefinitely all by itself.
Regarding depending on cassandra-all, In principle I don't have a strong preference... both seem kind of janky. In practice, I'd prefer sticking with the copy paste until it becomes a problem, just because it's done. I spent about 20 minutes trying to get the cassandra-all approach to work locally and the compiler couldn't find any of the classes we're using. I fiddled around with it a bit, but didn't want to spend too much time on it.
Finally, about suggestions regarding command stores. Let's revisit this when the metadata persistence patch is ready for review. A lot of what you're suggesting is already done in that patch in one form or another.
--
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-accord] bdeggleston commented on pull request #2: Initial Cassandra integration
Posted by GitBox <gi...@apache.org>.
bdeggleston commented on pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2#issuecomment-1083716847
I guess that's fair. It looks like this addresses more things introduced in this patch that I'd realized initially. Would you mind rebasing it onto this branch? Could we also leave the various CommandStore[s] changes in nested classes? This has been reworked for metadata persistence and that would simplify rebasing.
--
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-accord] bdeggleston commented on a change in pull request #2: Initial Cassandra integration
Posted by GitBox <gi...@apache.org>.
bdeggleston commented on a change in pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2#discussion_r838950423
##########
File path: accord-core/src/main/java/accord/messages/ReplyContext.java
##########
@@ -0,0 +1,5 @@
+package accord.messages;
+
+public interface ReplyContext
Review comment:
here you go: https://github.com/bdeggleston/cassandra/blob/accord-integration/src/java/org/apache/cassandra/service/accord/AccordMessageSink.java#L100
In C*, the reply context is the incoming message. After the netty refactor, the message is used to create the reply, and auto-fills stuff like the response verb, the id of course, and some timeout related stuff iirc
--
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-accord] belliottsmith commented on pull request #2: Initial Cassandra integration
Posted by GitBox <gi...@apache.org>.
belliottsmith commented on pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2#issuecomment-1085852532
Yep, rebased and pushed.
I think there remain some unaddressed comments from the last PR it would be great to close-out, perhaps we can address them in this PR and close that one so we can get to a single point to cleanly move forwards from?
I cannot find it now, also, but I remember commenting additionally about the `TxnRequest.Scope` and whether we actually need to send every `KeysForEpoch` or just the superset of `Keys` and the range of epochs we cross?
--
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-accord] belliottsmith commented on pull request #2: Initial Cassandra integration
Posted by GitBox <gi...@apache.org>.
belliottsmith commented on pull request #2:
URL: https://github.com/apache/cassandra-accord/pull/2#issuecomment-1070892131
I have some suggested improvements to how we manage command stores: https://github.com/belliottsmith/accord/tree/topology-integration-suggest
Also, I think this patch continues to have some issues passing the burn test
--
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