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

Update FATE transaction ids to be globally unique across multiple stores #4044

Closed
cshannon opened this issue Dec 8, 2023 · 5 comments
Closed
Assignees
Milestone

Comments

@cshannon
Copy link
Contributor

cshannon commented Dec 8, 2023

While working on changes for #3559 to store FATE operations inside an Accumulo Table, I realized that we will need some way to track FATE transactions globally. After we update FATE to store operations in Accumulo we still need the ZK store for FATE operations for the root/metadata tables (for example). Also we might want to have multiple Accumulo stores depending on how we want to design things such as storing operations for users separately from system operations in different tables.

Regardless of how the exact design turns out we are going to need multiple stores and right now FATE transactions are unique to a single store so we need some way to make sure things are unique. At the same time we could refactor the transaction id to be a better Id than just a long.

@keith-turner
Copy link
Contributor

This can help prevent bugs like creating a FATE operation in FATE instance A and trying to use the id in FATE instance B. Could also be useful for debugging, if when a FATE id is logged its easy to see what FATE instance the id came from.

@kevinrr888
Copy link
Member

I would like to work on this

kevinrr888 added a commit to kevinrr888/accumulo that referenced this issue Jan 24, 2024
A WIP for apache#4044. The end goal is to have the stronger type FateId replace the current representation of a transaction id (which is just a long). This was brought about from the addition of the AccumuloStore class - there are now two fate instance types associated with a transaction - META (for ZooStore) or USER (for AccumuloStore). FateId is a new class which includes the FateInstanceType and the transaction id.
Current changes:
- FateTxId replaced with new class FateId
- Started with changes in ReadOnlyFateStore and resolved the issues in other classes extending from changing this class.
Still left TODO:
- There are still related problems from the classes I have changed: I have not yet finished going down the entire chain of issues
- Need to change Fate and the associated issues with changing this class
- Need to change Repo and the associated issues with changing this class
- More changes TBD
keith-turner pushed a commit that referenced this issue Feb 1, 2024
The end goal is to have the stronger type FateId replace the current representation of a transaction id (which is just a long). This was brought about from the addition of the AccumuloStore class - there are now two fate instance types associated with a transaction - META (for ZooStore) or USER (for AccumuloStore). FateId is a new class which includes the FateInstanceType and the transaction id. This commit is for #4044
@kevinrr888
Copy link
Member

kevinrr888 commented Feb 1, 2024

The end goal is to have the stronger type FateId replace the current representation of a transaction id (which is just a long). This was brought about from the addition of the AccumuloStore class - there are now two fate instance types associated with a transaction - META (for ZooStore) or USER (for AccumuloStore). FateId is a new class which includes the FateInstanceType and the transaction id.

TODO list for this issue:

  • Create new class FateId to replace FateTxId
  • Change the stores to use FateId. Start with ReadOnlyFateStores. Resolve issues stemming from these changes.
  • Change Fate to use FateId. Resolve issues stemming from these changes.

(the above have been completed and merged in by PR#4191)

  • Change Repo to use FateId. Resolve issues stemming from these changes.

(the above has been completed and merged in by PR#4228)

  • CompactionConfigStorage, SelectedFiles, TabletUpdates, TabletMetadata, Ample need to be updated to use FateId. Deferred for now. All the places these classes are used have been marked "ELASTICITY_TODO DEFERRED - ISSUE 4044"

(the above has been completed and merged by PR#4247)

  • TabletOperationId, VolumeManager, TabletRefresher, TExternalCompactionJob, and Utils need to be updated to use FateId. Deferred for now. All the places these classes are used have been marked "ELASTICITY_TODO DEFERRED - ISSUE 4044"
  • A couple of deferred changes to Compactor and CompactionCoordinator (in PR#4258) (need PR#4247 merged first). Marked with "ELASTICITY_TODO DEFERRED - ISSUE 4044".

(the above has been completed and merged by PR#4258)

  • AdminUtil and Admin need to use FateId. Deferred for now. Marked with "ELASTICITY_TODO DEFERRED - ISSUE 4044". (issue#4168)

(the above has been completed and merged by PR#4350)

  • Delete FateTxId when all uses have been replaced with FateId (issue#4275)

(the above has been completed and merged by PR#4370)

Sorry, something went wrong.

@EdColeman
Copy link
Contributor

During this change, it would be desirable for the FATE transaction ids to nativity sort by creation timestamp. That would allow for determining order of operations of things solely by examining the FATE ids. This would apply to listing things in the fate store as well as looking at logs. If FATE_ID_1 < FATE_ID_2 then it can be immediately seen that FATE_ID_1 was created before FATE_ID_2 (for whatever timestamp precision is available)

One way to provide this could be to use UUIDs that conform to the emerging UUIDv7 standard. The gist of UUIDv7 - they are 128 bit UUIDs that put the timestamp portion of the UUID first, and random bits at the end with other identifying info in the middle. There are variants that are UUIDv7 compatible that allow for sub-second timing information if that precision is wanted.

kevinrr888 added a commit to kevinrr888/accumulo that referenced this issue Feb 8, 2024
This addresses several previously deferred changes for issue apache#4044. Root of most of these new changes were from changing TabletMetadata and TabletUpdates (in Ample) - FateId is now stored in the Metadata table instead of just the formatted long id. Addresses deferred changes to TabletMetadata, TabletUpdates, CompactionConfigStorage, SelectedFiles, and Ample.
kevinrr888 added a commit to kevinrr888/accumulo that referenced this issue Feb 12, 2024
This addresses several previously deferred changes for issue apache#4044.
Changes:
- ZooReservation now uses FateId (used in Utils)
- TabletOperationId now uses FateId
- TExternalCompactionJob now uses FateId
- VolumeManager and VolumeManagerImpl now use FateId
- Utils.getLock() lockData now uses the full FateId
- TabletRefresher now uses FateId
- Classes which used the above classes updated
- Several test changes to reflect new changes
- Deferred a couple of changes (in Compactor and CompactionCoordinator) (need pull/4247 merged first)
keith-turner pushed a commit that referenced this issue Feb 14, 2024
This addresses several previously deferred changes for issue #4044. Root of most of these new changes were from changing TabletMetadata and TabletUpdates (in Ample) - FateId is now stored in the Metadata table instead of just the formatted long id. Addresses deferred changes to TabletMetadata, TabletUpdates, CompactionConfigStorage, SelectedFiles, and Ample.
keith-turner pushed a commit that referenced this issue Feb 16, 2024
This addresses several previously deferred changes for issue #4044.
Changes:
- ZooReservation now uses FateId (used in Utils)
- TabletOperationId now uses FateId
- TExternalCompactionJob now uses FateId
- VolumeManager and VolumeManagerImpl now use FateId
- Utils.getLock() lockData now uses the full FateId
- TabletRefresher now uses FateId
- Classes which used the above classes updated
- Several test changes to reflect new changes
- Deferred a couple of changes (in Compactor and CompactionCoordinator) (need pull/4247 merged first)
@kevinrr888
Copy link
Member

All above TODOs have been completed. I believe this issue can be closed now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

5 participants