apache / accumulo

Apache Accumulo
https://accumulo.apache.org
Apache License 2.0
1.07k stars 445 forks source link

Replace AbstractFateStore.createDummyLockID() #4904

Open kevinrr888 opened 2 months ago

kevinrr888 commented 2 months ago

Is your feature request related to a problem? Please describe. From the changes from https://github.com/apache/accumulo/pull/4524/, reserving a transaction now indicates it is reserved with a FateReservation object which has a LockID. From using the stores from the context of the Manager, this is needed for identifying reservations held by a dead Manager so they can be unreserved. However, the stores are not always used in the context of a Manager. One example is testing. As it is right now, whenever a store needs to make a reservation, a LockID is required. The LockID only makes sense in the context of the Manager, so currently a dummy lock is made when a store is created outside of the context of a Manager. This dummy lock exists so reservations can still be made.

Describe the solution you'd like A path could be created in ZK for utilities to get a ZK lock (proposed by @keith-turner in https://github.com/apache/accumulo/pull/4524#discussion_r1631626836). This could replace the AbstractFateStore.createDummyLockID() method. This could also be used to change some of the admin fate commands (those that require the Manager to be down: fail and delete) to no longer require the Manager to be down. Can instead get a LockID at this utility path and reserve the transaction they operate on instead of requiring the Manager to be down. At the same time working on this, if the store is used in a readonly way, then there is no need for a LockID as it will never reserve. In the changes in https://github.com/apache/accumulo/pull/4524/, a dummy lock is created and passed to the store for all use cases of the store outside of the Manager. This should be looked at more closely and only use a LockID if the store will be used to write data. If the store is not expected to write, it should fail on write operations, which it won't currently.

So, the changes/solution are: