Enables locking on an arbitrary lockID #164
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,28 @@ public void acquireLock(final ScheduledJobParameter jobParameter, | |
| final JobExecutionContext context, ActionListener<LockModel> listener) { | ||
| final String jobIndexName = context.getJobIndexName(); | ||
| final String jobId = context.getJobId(); | ||
| String lockId = LockModel.generateLockId(jobIndexName, jobId); | ||
| acquireLockWithId(jobParameter, context, lockId, listener); | ||
| } | ||
|
|
||
| /** | ||
| * Attempts to acquire a lock with a specific lock Id. If the lock does not exist it attempts to create the lock document. | ||
| * If the Lock document exists, it will try to update and acquire the lock. | ||
| * | ||
| * @param jobParameter a {@code ScheduledJobParameter} containing the lock duration. | ||
| * @param context a {@code JobExecutionContext} containing job index name and job id. | ||
| * @param lockId the unique Id for the lock. This should represent the resource that the lock is on, whether it be | ||
| * a job, or some other arbitrary resource. | ||
| * @param listener an {@code ActionListener} that has onResponse and onFailure that is used to return the lock if it was acquired | ||
| * 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 |
||
| final JobExecutionContext context, | ||
| final String lockId, | ||
| ActionListener<LockModel> listener) { | ||
| final String jobIndexName = context.getJobIndexName(); | ||
| final String jobId = context.getJobId(); | ||
| if (jobParameter.getLockDurationSeconds() == null) { | ||
| listener.onFailure(new IllegalArgumentException("Job LockDuration should not be null")); | ||
| } else { | ||
|
|
@@ -114,7 +136,7 @@ public void acquireLock(final ScheduledJobParameter jobParameter, | |
| created -> { | ||
| if (created) { | ||
| try { | ||
| findLock(lockId, ActionListener.wrap( | ||
| existingLock -> { | ||
| if (existingLock != null) { | ||
| if (isLockReleasedOrExpired(existingLock)) { | ||
|
|
@@ -130,7 +152,7 @@ public void acquireLock(final ScheduledJobParameter jobParameter, | |
| } | ||
| } else { | ||
| // There is no lock object and it is first time. Create new lock. | ||
| LockModel tempLock = new LockModel(jobIndexName, jobId, lockId, getNow(), | ||
| lockDurationSecond, false); | ||
| logger.debug("Lock does not exist. Creating new lock" + tempLock); | ||
| createLock(tempLock, listener); | ||
|
|
@@ -227,7 +249,7 @@ private void findLock(final String lockId, ActionListener<LockModel> listener) { | |
| .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, | ||
| response.getSourceAsString()); | ||
| parser.nextToken(); | ||
| listener.onResponse(LockModel.parseWithID(parser, response.getSeqNo(), response.getPrimaryTerm(), response.getId())); | ||
| } catch (IOException e) { | ||
| logger.error("IOException occurred finding lock", e); | ||
| listener.onResponse(null); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we want both jobId and lockId?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.