fix(server): fix tasks state handler logic #987
Conversation
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| { lockedAt: { $lt: new Date(Date.now() - 1000 * this.lockTimeout) } }, | ||
| { $set: { lockedAt: TASK_LOCK_INIT_TIME } }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
!
Code Review:
- In the handleDeletedState method, the clearTimeoutLocks method is removed which I think is not necessary.
- In the handleCreatedState method, if (!res.value) return is added which looks good.
- In the handleDeletedState method, if (updated.modifiedCount > 0) this.logger.debug('app phase updated to
Deleted:', app.appid) is replaced with this.logger.log('app phase updated toDeleted: ' + app.appid). This is good as it is more easier to understand. - In the clearDeletedApps method, the TODO comment should be removed and the state should be set to ApplicationState.Deleted.
- In the unlockApplication method, the lockedAt variable should be set to TASK_LOCK_INIT_TIME instead of 0.
- In the clearTimeoutLocks method, the lockedAt variable should be set to TASK_LOCK_INIT_TIME instead of 0.
Hope this helps.
| @@ -283,6 +285,7 @@ export class WebsiteTaskService { | |||
| { | |||
| state: DomainState.Deleted, | |||
| phase: DomainPhase.Created, | |||
| lockedAt: { $lt: new Date(Date.now() - 1000 * this.lockTimeout) }, | |||
| }, | |||
| { | |||
| $set: { | |||
There was a problem hiding this comment.
with the code review
The patch looks good and there is no bug risk. The updateMany() method has been updated to include two new conditions: $in, which checks for multiple values; and $lt, which checks for a lockedAt value that is less than a certain time interval. Both of these conditions should help improve the accuracy and performance of the query. Additionally, the lockTimeout variable has been added in order to set a reasonable timeout limit for the query.
| if (!namespace) { | ||
| return | ||
| } | ||
| if (!namespace) return | ||
|
|
||
| const appsV1Api = this.clusterService.makeAppsV1Api(region) | ||
| const coreV1Api = this.clusterService.makeCoreV1Api(region) |
There was a problem hiding this comment.
off with a code review!
First, we can see that the original code was trying to check for a namespace before continuing. However, the updated code simply returns if there is no namespace, so it appears the condition will be more efficiently handled.
We can also note that the code is making use of the ClusterService class to make an appsV1Api and coreV1Api which implies that those objects are needed for further processing. It would be worth verifying this is the case to ensure the intended functionality is preserved.
Finally, there are no obvious bugs in the code. However, it would be worth considering if there is any additional error handling that should be included in case something unexpected occurs.
| state: DomainState.Deleted, | ||
| phase: { $in: [DomainPhase.Created, DomainPhase.Creating] }, | ||
| lockedAt: { $lt: new Date(Date.now() - 1000 * this.lockTimeout) }, | ||
| }, | ||
| { | ||
| $set: { phase: DomainPhase.Deleting, lockedAt: TASK_LOCK_INIT_TIME }, | ||
| }, |
There was a problem hiding this comment.
with code review
First, the code patch is intended to update the document in the 'StorageBucket' collection in the SystemDatabase with conditions based on the state and phase of the domain.
The code patch adds a condition - lockedAt - to the query used to locate the documents that need to be updated. The condition checks if the lockedAt of the document is less than the current date minus 1000 times the value of lockTimeout. This condition ensures that only documents that have been locked for longer than the timeout are selected for update.
Another thing to note is that the value of the 'phase' field is being set to different values depending on the initial state and phase of the document. This ensures that the document is updated to the correct state.
Overall, the code patch looks correct and should work as expected. However, it is recommended to add tests to ensure that the code is doing what it is supposed to do.
| { | ||
| state: TriggerState.Inactive, | ||
| phase: { $in: [TriggerPhase.Created, TriggerPhase.Creating] }, | ||
| }, | ||
| { | ||
| $set: { phase: TriggerPhase.Deleting, lockedAt: TASK_LOCK_INIT_TIME }, | ||
| }, |
There was a problem hiding this comment.
with code review:
1.It looks like the code is attempting to update the state of a CronTrigger to Inactive, when it is in either the Created or Creating phase.
2.The code is also setting the lockedAt field to the TASK_LOCK_INIT_TIME.
3.It appears that the code is properly using the SystemDataBase to update the data and is not using any deprecated functions.
4.The code should be tested to ensure that it is working as expected.
5.It looks like the code is missing an 'import assert from 'node:assert' statement. This should be added in order for the code to work properly.
No description provided.