Async jobs add endtime #2739
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -161,7 +161,7 @@ public ConfigKey<?>[] getConfigKeys() { | |
|
|
||
| @Override | ||
| public AsyncJobVO getAsyncJob(long jobId) { | ||
| return _jobDao.findByIdIncludingRemoved(jobId); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -286,9 +286,9 @@ public void completeAsyncJob(final long jobId, final Status jobStatus, final int | |
| if (s_logger.isDebugEnabled()) { | ||
| s_logger.debug("Wake up jobs related to job-" + jobId); | ||
| } | ||
| final List<Long> wakeupList = Transaction.execute(new TransactionCallback<List<Long>>() { | ||
| @Override | ||
| public List<Long> doInTransaction(final TransactionStatus status) { | ||
| if (s_logger.isDebugEnabled()) { | ||
| s_logger.debug("Update db status for job-" + jobId); | ||
| } | ||
|
|
@@ -302,14 +302,16 @@ public List<Long> doInTransaction(TransactionStatus status) { | |
| job.setResult(null); | ||
| } | ||
|
|
||
| final Date currentGMTTime = DateUtil.currentGMTTime(); | ||
| job.setLastUpdated(currentGMTTime); | ||
| job.setRemoved(currentGMTTime); | ||
| job.setExecutingMsid(null); | ||
| _jobDao.update(jobId, job); | ||
|
|
||
| if (s_logger.isDebugEnabled()) { | ||
| s_logger.debug("Wake up jobs joined with job-" + jobId + " and disjoin all subjobs created from job- " + jobId); | ||
| } | ||
| final List<Long> wakeupList = wakeupByJoinedJobCompletion(jobId); | ||
| _joinMapDao.disjoinAllJobs(jobId); | ||
|
|
||
| // purge the job sync item from queue | ||
|
|
@@ -445,8 +447,8 @@ public void syncAsyncJobExecution(AsyncJob job, String syncObjType, long syncObj | |
| } | ||
|
|
||
| @Override | ||
| public AsyncJob queryJob(final long jobId, final boolean updatePollTime) { | ||
| final AsyncJobVO job = _jobDao.findByIdIncludingRemoved(jobId); | ||
|
|
||
| if (updatePollTime) { | ||
| job.setLastPolled(DateUtil.currentGMTTime()); | ||
|
|
@@ -1025,21 +1027,24 @@ public void doInTransactionWithoutResult(TransactionStatus status) { | |
| // purge sync queue item running on this ms node | ||
| _queueMgr.cleanupActiveQueueItems(msid, true); | ||
| // reset job status for all jobs running on this ms node | ||
| final List<AsyncJobVO> jobs = _jobDao.getResetJobs(msid); | ||
| for (final AsyncJobVO job : jobs) { | ||
| if (s_logger.isDebugEnabled()) { | ||
| s_logger.debug("Cancel left-over job-" + job.getId()); | ||
| } | ||
| job.setStatus(JobInfo.Status.FAILED); | ||
| job.setResultCode(ApiErrorCode.INTERNAL_ERROR.getHttpCode()); | ||
| job.setResult("job cancelled because of management server restart or shutdown"); | ||
| job.setCompleteMsid(msid); | ||
| final Date currentGMTTime = DateUtil.currentGMTTime(); | ||
| job.setLastUpdated(currentGMTTime); | ||
| job.setRemoved(currentGMTTime); | ||
| _jobDao.update(job.getId(), job); | ||
| if (s_logger.isDebugEnabled()) { | ||
| s_logger.debug("Purge queue item for cancelled job-" + job.getId()); | ||
| } | ||
| _queueMgr.purgeAsyncJobQueueItemId(job.getId()); | ||
| if (ApiCommandJobType.Volume.toString().equals(job.getInstanceType())) { | ||
|
|
||
| try { | ||
| _volumeDetailsDao.removeDetail(job.getInstanceId(), "SNAPSHOT_ID"); | ||
|
|
@@ -1049,8 +1054,8 @@ public void doInTransactionWithoutResult(TransactionStatus status) { | |
| } | ||
| } | ||
| } | ||
| final List<SnapshotDetailsVO> snapshotList = _snapshotDetailsDao.findDetails(AsyncJob.Constants.MS_ID, Long.toString(msid), false); | ||
|
Member
There was a problem hiding this comment. Why are using this final? There are other places you are adding them, what is the real benefit? Besides adding more words for us to read.
Member
There was a problem hiding this comment. I think it's a good defensive programming style to keep assignment restrictions unless needed.
Member
There was a problem hiding this comment. Sorry if I sound blunt, but yes I know this "excuse". My question is, what are the cases that the use of these final keywords is helping us to avoid? Is it the case that we have these monstrous methods (with 100+, or 500+ lines)? And then you are trying to help programmers to identify when he/she is changing a variable that was previously assigned somewhere else in the middle of a big chunk of code. If that is the only reason for us to use this keyword, I do prefer other options such as using shorter and concise methods, which can be unit tested. Then, we can catch problems such as wrong assignments. Otherwise, we are just adding code that is not needed.
Contributor
Author
There was a problem hiding this comment. Best practice is to make objects immutable and it makes the code safer and more predictable. I agree that the methods should be much shorter in the first place.
Contributor
There was a problem hiding this comment. Actually it is kind of kidding yourself in this case, as the object ref may not be changed after setting but the state can still be changed. I would advice using a final filter against this as a spam filter against spam; ignore, don't read.
Member
There was a problem hiding this comment. That is exactly my point. I have seen this indiscriminate use of the I really do not understand when I see people slapping |
||
| for (final SnapshotDetailsVO snapshotDetailsVO : snapshotList) { | ||
| SnapshotInfo snapshot = snapshotFactory.getSnapshot(snapshotDetailsVO.getResourceId(), DataStoreRole.Primary); | ||
| snapshotSrv.processEventOnSnapshotObject(snapshot, Snapshot.Event.OperationFailed); | ||
| _snapshotDetailsDao.removeDetail(snapshotDetailsVO.getResourceId(), AsyncJob.Constants.MS_ID); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package com.cloud.storage.dao; | ||
|
|
||
| import com.cloud.api.query.dao.AsyncJobJoinDaoImpl; | ||
| import com.cloud.api.query.vo.AsyncJobJoinVO; | ||
| import org.apache.cloudstack.api.ApiCommandJobType; | ||
| import org.apache.cloudstack.api.response.AsyncJobResponse; | ||
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.mockito.InjectMocks; | ||
| import org.mockito.runners.MockitoJUnitRunner; | ||
| import org.springframework.test.util.ReflectionTestUtils; | ||
|
|
||
| import java.util.Date; | ||
|
|
||
| @RunWith(MockitoJUnitRunner.class) | ||
| public class AsyncJobJoinDaoTest { | ||
|
|
||
| @InjectMocks | ||
| AsyncJobJoinDaoImpl dao; | ||
|
|
||
| @Test | ||
| public void testNewAsyncJobResponseValidValues() { | ||
| final AsyncJobJoinVO job = new AsyncJobJoinVO(); | ||
| ReflectionTestUtils.setField(job,"uuid","a2b22932-1b61-4406-8e89-4ae19968e8d3"); | ||
| ReflectionTestUtils.setField(job,"accountUuid","4dea2836-72cc-11e8-b2de-107b4429825a"); | ||
| ReflectionTestUtils.setField(job,"domainUuid","4dea136b-72cc-11e8-b2de-107b4429825a"); | ||
| ReflectionTestUtils.setField(job,"userUuid","4decc724-72cc-11e8-b2de-107b4429825a"); | ||
| ReflectionTestUtils.setField(job,"cmd","org.apache.cloudstack.api.command.admin.vm.StartVMCmdByAdmin"); | ||
| ReflectionTestUtils.setField(job,"status",0); | ||
| ReflectionTestUtils.setField(job,"resultCode",0); | ||
| ReflectionTestUtils.setField(job,"result",null); | ||
| ReflectionTestUtils.setField(job,"created",new Date()); | ||
| ReflectionTestUtils.setField(job,"removed",new Date()); | ||
| ReflectionTestUtils.setField(job,"instanceType",ApiCommandJobType.VirtualMachine); | ||
| ReflectionTestUtils.setField(job,"instanceId",3L); | ||
| final AsyncJobResponse response = dao.newAsyncJobResponse(job); | ||
| Assert.assertEquals(job.getUuid(),response.getJobId()); | ||
| Assert.assertEquals(job.getAccountUuid(), ReflectionTestUtils.getField(response, "accountId")); | ||
| Assert.assertEquals(job.getUserUuid(), ReflectionTestUtils.getField(response, "userId")); | ||
| Assert.assertEquals(job.getCmd(), ReflectionTestUtils.getField(response, "cmd")); | ||
| Assert.assertEquals(job.getStatus(), ReflectionTestUtils.getField(response, "jobStatus")); | ||
| Assert.assertEquals(job.getStatus(), ReflectionTestUtils.getField(response, "jobProcStatus")); | ||
| Assert.assertEquals(job.getResultCode(), ReflectionTestUtils.getField(response, "jobResultCode")); | ||
| Assert.assertEquals(null, ReflectionTestUtils.getField(response, "jobResultType")); | ||
| Assert.assertEquals(job.getResult(), ReflectionTestUtils.getField(response, "jobResult")); | ||
| Assert.assertEquals(job.getInstanceType().toString(), ReflectionTestUtils.getField(response, "jobInstanceType")); | ||
| Assert.assertEquals(job.getInstanceUuid(), ReflectionTestUtils.getField(response, "jobInstanceId")); | ||
| Assert.assertEquals(job.getCreated(), ReflectionTestUtils.getField(response, "created")); | ||
| Assert.assertEquals(job.getRemoved(), ReflectionTestUtils.getField(response, "removed")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testNewAsyncJobResponseNullValues() { | ||
| final AsyncJobJoinVO job = new AsyncJobJoinVO(); | ||
| final AsyncJobResponse response = dao.newAsyncJobResponse(job); | ||
| Assert.assertEquals(job.getUuid(),response.getJobId()); | ||
| Assert.assertEquals(job.getAccountUuid(), ReflectionTestUtils.getField(response, "accountId")); | ||
|
Contributor
There was a problem hiding this comment. very good, not assuming the nulls ;) |
||
| Assert.assertEquals(job.getUserUuid(), ReflectionTestUtils.getField(response, "userId")); | ||
| Assert.assertEquals(job.getCmd(), ReflectionTestUtils.getField(response, "cmd")); | ||
| Assert.assertEquals(job.getStatus(), ReflectionTestUtils.getField(response, "jobStatus")); | ||
| Assert.assertEquals(job.getStatus(), ReflectionTestUtils.getField(response, "jobProcStatus")); | ||
| Assert.assertEquals(job.getResultCode(), ReflectionTestUtils.getField(response, "jobResultCode")); | ||
| Assert.assertEquals(null, ReflectionTestUtils.getField(response, "jobResultType")); | ||
| Assert.assertEquals(job.getResult(), ReflectionTestUtils.getField(response, "jobResult")); | ||
| Assert.assertEquals(job.getInstanceType(), ReflectionTestUtils.getField(response, "jobInstanceType")); | ||
| Assert.assertEquals(job.getInstanceUuid(), ReflectionTestUtils.getField(response, "jobInstanceId")); | ||
| Assert.assertEquals(job.getCreated(), ReflectionTestUtils.getField(response, "created")); | ||
| Assert.assertEquals(job.getRemoved(), ReflectionTestUtils.getField(response, "removed")); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So, the column already existed but we were not using it?
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.
yes