Enables locking on an arbitrary lockID #164
Conversation
Signed-off-by: Clay Downs <downsrob@amazon.com>
Signed-off-by: Clay Downs <downsrob@amazon.com>
Signed-off-by: Clay Downs <downsrob@amazon.com>
Signed-off-by: Clay Downs <downsrob@amazon.com>
Signed-off-by: Clay Downs <downsrob@amazon.com>
| * or else null. Passes {@code IllegalArgumentException} to onFailure if the {@code ScheduledJobParameter} does not | ||
| * have {@code LockDurationSeconds}. | ||
| */ | ||
| public void acquireLockWithId(final ScheduledJobParameter jobParameter, |
There was a problem hiding this comment.
If given a lockId by the caller, I think we should still prefix it with the job index name to preserve namespaces, otherwise if two different plugins using this end up using the same lockId at any point then they'll run into issues.
There was a problem hiding this comment.
I am happy to make this change if you want, but that behavior also enables locking resources across plugins. Any plugin that doesn't want this could just prefix their own id to namespace it.
There was a problem hiding this comment.
Is there a use case for that yet? If not then we can add in support for it when needed, but as of right now relying on the consumers to correctly namespace the lockId leaves room for the developers who are not aware of it to accidentally misuse and cause issues across plugins.
@thalurur Any thoughts on it?
There was a problem hiding this comment.
I don't see much benefit in enforcing a global lock since this is not a standard that all plugins need to follow and one plugin assuming that its locking globally using this mechanism might be really not enforcable and I don't think a plugin should assume that.
Keeping it at the a namespace always by job scheduler makes more sense since its contained to that use case and if plugins want to further scope it down they can add more prefixes for the feature in the namespace when creating lock Ids
| @@ -88,6 +94,18 @@ public LockModel(String jobIndexName, String jobId, Instant lockTime, | |||
| this.primaryTerm = primaryTerm; | |||
| } | |||
|
|
|||
| public LockModel(String jobIndexName, String jobId, String lockId, Instant lockTime, | |||
There was a problem hiding this comment.
why do we want both jobId and lockId?
There was a problem hiding this comment.
That's a good point. I think that the only reason to include the job ID would be if we wanted to delete the leftover locks after a job is deleted. If we instead moved to a periodic job deleting stale locks, then I think there is no point in having both. I'll make the change.
Signed-off-by: Clay Downs <downsrob@amazon.com>
Signed-off-by: Clay Downs <downsrob@amazon.com>
| @@ -80,6 +80,8 @@ public LockModel(String jobIndexName, String jobId, Instant lockTime, | |||
| long lockDurationSeconds, boolean released, long seqNo, long primaryTerm) { | |||
| this.lockId = jobIndexName + LOCK_ID_DELIMITR + jobId; | |||
| this.jobIndexName = jobIndexName; | |||
| // The jobId parameter does not necessarily need to represent the id of a job scheduler job, as it is being used | |||
There was a problem hiding this comment.
may be create an issue in repo to rename this completely to just lockId everywhere? so the paradigm is changed in job scheduler - cleaning up stale locks and any other refactor needed with this change
* Enables locking on an arbitrary lockID Signed-off-by: Clay Downs <downsrob@amazon.com>
Signed-off-by: Clay Downs downsrob@amazon.com
Description
This change allows for locking an arbitrary resource across jobs by repurposing the LockModel job_id to be any user provided lock ID String. The locks are still scoped to the specific job index provided.
Previously, locks could only be acquired by supplying the ScheduledJobParameter and JobExecutionContext. The job index name, job ID, and lock duration would then be extracted from those objects. This change adds a method to acquire a lock with any provided lock ID, and allows for explicitly providing the job index name and lock duration, so the ScheduledJobParameter and JobExecutionContext are no longer required.
Issues Resolved
#166
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.