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

Add Load-balancing cookie support #206

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

AnupamaSarjoshi
Copy link
Contributor

@AnupamaSarjoshi AnupamaSarjoshi commented Mar 18, 2024

Hi Richard,

As discussed in the forum https://coderunner.org.nz/mod/forum/discuss.php?d=667#p2726, we have implemented the fix
to support the Jobe back-ends that use cookies for sticky load-balancing, by creating a single curl object with support for cookies, and using it for all requests.

Could you please review? Have tested this by running the Jobe server on our AWS test environment, with load balancer generated cookies.

Thanks,
Anupama

These changes support AWS load-balancer stickiness,
by saving and sending back the cookie set by the load balancer
using curl.
@trampgeek trampgeek merged commit d7ba97b into trampgeek:master Mar 20, 2024
5 checks passed
@trampgeek
Copy link
Owner

Thanks Anupama. Another nice contribution.

@trampgeek
Copy link
Owner

Hi Anupama. We're getting log messages on the unlink($cookiefile) line saying the file doesn't exist. So presumably the fix is to prefix the unlink with '@'?

@timhunt
Copy link
Collaborator

timhunt commented Mar 22, 2024

In my bitter experience, any time you hide an error with @, you later regret it when you have a bug which is really hard to debug.

However, unlink is one of the safest times to do that, and it seems really common in other Moodle code, so I agree with your suggestion @trampgeek.

(A few places in Moodle do if (file_exists(...)) { unlink(...); } intead, but they are the exception.)

@AnupamaSarjoshi
Copy link
Contributor Author

Thanks Richard and Tim for the fix. Sounds good :)
But the strange thing is we didn't get those messages in our logs - checked on both my local and our test environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants