Short Description
Either merge the DistributedLockProvider interface functions into the StorageProvider interface or remove duplicate functions from `DistributedLockProvider).
Definition of Done
Either:
StorageProvider has the GetDuration(), DistributedTimedLock(), and Unlock() functions currently in DistributedLockProvider. The DistributedLockProvider interface has been removed and functions that accept it now use StorageProvider instead.
DistributedLockProvider no longer includes Init(), InitFromStorage(), or Ping() functions.
My preference is to merge the interfaces.
Additional context
AFAICT DistributedLockProvider has no reason to be separate. There are no StorageProvider implementations that are not also DistributedLockProvider implementations, and I do not expect we will ever have reason to implement a StorageProvider only in the future.
At present their separation leads to unnecessary duplication of Init() and Ping() code and the weird InitFromStorage() partial copy via prayer and abuse of interface{} method.
If the interfaces remain separate, we'll need to remove some calls to Ping() that are superfluous (the functions also ping the storage engine already).
If we expect implementations that do not implement distributed locks (for a either single PCS instance or PCS instance groups with only one leader), we can make the distributed lock functions no-ops in those implementations.
On further review, we have no need for GetDuration(); it should be removed regardless. It's only used in its own test, and provides no functionality other than echoing an ephemeral function parameter(a lock grab timeout).
Short Description
Either merge the
DistributedLockProviderinterface functions into theStorageProviderinterface or remove duplicate functions from `DistributedLockProvider).Definition of Done
Either:
StorageProviderhas the,GetDuration()DistributedTimedLock(), andUnlock()functions currently inDistributedLockProvider. TheDistributedLockProviderinterface has been removed and functions that accept it now useStorageProviderinstead.DistributedLockProviderno longer includesInit(),InitFromStorage(), orPing()functions.My preference is to merge the interfaces.
Additional context
AFAICT
DistributedLockProviderhas no reason to be separate. There are noStorageProviderimplementations that are not alsoDistributedLockProviderimplementations, and I do not expect we will ever have reason to implement aStorageProvideronly in the future.At present their separation leads to unnecessary duplication of
Init()andPing()code and the weirdInitFromStorage()partial copy via prayer and abuse ofinterface{}method.If the interfaces remain separate, we'll need to remove some calls to
Ping()that are superfluous (the functions also ping the storage engine already).If we expect implementations that do not implement distributed locks (for a either single PCS instance or PCS instance groups with only one leader), we can make the distributed lock functions no-ops in those implementations.
On further review, we have no need for
GetDuration(); it should be removed regardless. It's only used in its own test, and provides no functionality other than echoing an ephemeral function parameter(a lock grab timeout).