Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve concurrency behavior of Flotilla #1159

Merged
merged 10 commits into from
Nov 14, 2023
Merged

Conversation

aeshub
Copy link
Contributor

@aeshub aeshub commented Nov 12, 2023

No description provided.

@aeshub aeshub added the bug Something isn't working label Nov 12, 2023
@aeshub aeshub self-assigned this Nov 12, 2023
@aeshub aeshub force-pushed the stop-button-fix branch 2 times, most recently from 83174d6 to af22071 Compare November 13, 2023 09:59
@aeshub aeshub changed the title Stop button fix Improve concurrency behavior of Flotilla Nov 13, 2023
@aeshub aeshub force-pushed the stop-button-fix branch 2 times, most recently from 39b4c6c to 9b0c41a Compare November 13, 2023 14:25
Copy link
Contributor

@andchiind andchiind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only minor comments and questions, but it could be good to answer the one about using 3 requests instead of 1 before merging.

continue;
}

missionRun.Status = MissionStatus.Pending;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have a SetCurrentMissionId in robotService, we could maybe consider having SetCurrentMissionStatus in missionRunService. But if this code already works consistently then this is not needed I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. Although not added as part of this PR.

@@ -221,8 +221,20 @@ private async Task MoveInterruptedMissionsToQueue(IEnumerable<string> interrupte
continue;
}

missionRun.Status = MissionStatus.Pending;
await _missionRunService.Update(missionRun);
var newMissionRun = new MissionRun
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this change maybe be rebased into the first commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. However, the merge conflicts that arose were sufficiently difficult that I decided against it.

{
await _robotService.UpdateRobotStatus(robot.Id, RobotStatus.Busy);
await _robotService.UpdateCurrentMissionId(robot.Id, missionRun.Id);
await _robotService.UpdateCurrentArea(robot.Id, area);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safer to do in 3 requests instead of 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to do in one as we have additional database reads and writes although I don't think the difference is large as we have the semaphore now. I didn't want to make too many combination functions of type UpdateRobotStatusAndCurrentMissionId....

For now will leave it like this and then we can consider improving it.

@@ -127,6 +134,15 @@ public async Task<Robot> Update(Robot robot)
return robot;
}

public async Task<IList<Robot>> ReadLocalizedRobotsForInstallation(string installationCode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but if we keep adding more service functions like these then we could perhaps make a generic filter function like we did for missionRun and missionDefinition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be relevant although the filter functions for mission run and mission definition are more linked to there being a lot of them and there is a need to be able to search specifically (for example by date etc.). As long as there are relatively few read functions for robots I think it's fine.

In my opinion this is also a lot more readable than having one function which we send a filter into. Easier to have a good, readable function name this way.

@aeshub aeshub merged commit 591c5c3 into equinor:main Nov 14, 2023
6 checks passed
@aeshub aeshub deleted the stop-button-fix branch November 14, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants