Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IGNITE-24384 Support write intent switches within colocation logic #5241

Merged
merged 23 commits into from
Feb 21, 2025

Conversation

rpuch
Copy link
Contributor

@rpuch rpuch commented Feb 17, 2025

* @param commit {@code true} if a commit requested.
* @param commitTimestamp Commit timestamp ({@code null} if it's an abort).
* @param txId Transaction id.
* @return Completable future of Void.
*/
CompletableFuture<Void> cleanup(
TablePartitionId commitPartitionId,
Collection<ReplicationGroupId> enlistedPartitions,
Collection<EnlistedPartitionGroup> enlistedPartitions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many similar entities MutablePartitionEnlistment, FinishingPartitionEnlistment, EnlistedPartitionGroup with some carelessness in javadocs:

  • EnlistedPartitionGroup javadoc claims that it's Partition enlistment together with partition group ID, meaning tableIds = Partition enlistment.
  • MutablePartitionEnlistment is primaryNode + consistencyToken + tableIds

Do you have any ideas of how to make MutablePartitionEnlistment, FinishingPartitionEnlistment, EnlistedPartitionGroup cleaner, probably decreasing the number of classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 3 represent similar information ('partition enlistment information') at different times in different contexts. They all relate to some groupId (but in 2 cases groupId is stored externally, as map key, and in the last case it's stored inside) and they all have tableIds (in first case it's mutable, in other cases it's not).

  1. MutablePartitionEnlistment (which I've just renamed to OngoingTxPartitionEnlistment to make it more clear what it is) is used to track enlistment information for a partition (which groupId is stored externally) while the transaction is still running, before it is finished; so it needs to know its primary replica (in the form of ClusterNode, not just name) and enlistment token
  2. FinishingPartitionEnlistment represents the same information when the transaction is being finished and when tx cleanup happens. At this stage, tableIds doesn't need to be mutable anymore, and token is not needed; the primary node is still needed but it can be represented as a name.
  3. EnlistedPartitionGroup represents this 'thing' stored in TxMeta (and in tx state storage). Primary information is not needed anymore, but groupId has to be stored explicitly. Even if we represented collections of EnlistedPartitionGroup as maps from groupIds to the remaining enlistment information, we wouldn't be able to reuse FinishingPartitionEnlistment because we don't primary node name it contains. (Technically, we could, but it would be ugly). On the other hand, I don't look the idea to remove EnlistedPartitionGroup and just use maps from groupIds to tableIds everywhere we use collections of EnlistedPartitionGroup (this would be hide an entity)

To sum up: I wasn't able to invent any way to remove any of these entities. I named them to make first and second look alike (as they are really close) and third to be a little different (as it's context is a bit different). If you have any ideas on how to improve this, please share them.

I also updated javadocs on the classes to decrease possible confusion.

Copy link
Contributor

@sanpwc sanpwc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[removed]

@rpuch rpuch merged commit d499093 into apache:main Feb 21, 2025
1 check passed
@rpuch rpuch deleted the ignite-24384 branch February 21, 2025 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants