-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement queuing priority #56 #58
Conversation
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.
This looks great! I love the concept of being able to prioritise different types of request based on who the user is and this is exactly what we will need once open data becomes a thing.
Unless I'm missing something the prioritisation is only applied to downloads that are put in the queue via the new Queue Visit and Queue Files endpoints. What I think we probably need to move to is a system where ALL downloads go via the queue and are prioritised but that will need a bit more thinking about. For example, the new recall methods need to have less priority as they are likely to be larger requests even though they are likely to be submitted by Instrument Scientists who may have higher priority. And where do we prioritise those against recalls from open data users? However, I think this is probably changes that we can make in the future, depending on what ideas you have on this.
if (authenticatedPriority < 1 && defaultPriority >= 1) { | ||
String msg = "queue.priority.authenticated disabled with value " + authenticatedString; | ||
msg += " but queue.priority.default enabled with value " + defaultPriority; | ||
msg += "\nAuthenticated users will use default priority if no superseding priority applies"; |
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.
These last 3 lines in both the if and the else could go below the if/else as they are common.
Although you would also need to initialise msg above the if/else, so that line is borderline worth/not worth it.
However, the final two lines are probably worth moving.
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.
Have refactored the last three lines, this also means introducing an else
clause so we can return early don't warn/set default when we don't meet either criteria (corresponding to a "normal" value for authenticatedPriority
).
Yes, this is currently unique to the polling which moves from
Agreed - I wouldn't want to start changing the existing workflow (other than e.g. for checking feasibility) without getting some buy in from the others (frontend/service management/DLS themselves).
Yeah it's not obvious to me what we want to enforce. Practically, I think the current system could be extended to cover some of this. For example, the reason for having a different |
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.
Apologies I didn't take in to account the fact that an else would be needed to cater for a direct return from the method. Having seen the updated method side by side with the old one, I prefer the original! Although it has the repeated lines of code it is simpler and easier to follow. So please just revert this commit and I'll approve the PR.
Also includes changes from #45 #51, due to dependency of priority settings on submitting to and polling the queue. See those PRs for those changes in isolation.
Closes #56