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

Allow to address a second replica DB "replica2" in the web code #5695

Closed
wants to merge 6 commits into from

Conversation

bema-aei
Copy link
Contributor

@bema-aei bema-aei commented Jul 18, 2024

The current server setup of Einstein@Home uses two replicas. This patch is to allow accessing the second replica in the web code by calling get_aux(2); get_aux(1) or get_aux(true) still addresses the standard replica.

For this to work, config.xml needs to have a second set of tags "<replica2_db_(host|name|user|password)>" analog to the current "<replica_db_(host|name|user|password)>".

This code will probably never be used by anyone else, it's just to bring upstream BOINC in sync with the web code that we're using on E@H for easier updates.

@brevilo brevilo self-requested a review July 18, 2024 13:16
Copy link
Contributor

@brevilo brevilo left a comment

Choose a reason for hiding this comment

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

There seem to be a few logic issues in get_aux(), at least when comparing code vs. comments. Eg. original lines:

  • 39 vs. 60: should not error out for fallback_mode = 1
  • 69 vs. 75: the else block doesn't imply fallback_mode == 0, if (!$u || !$p || !$n) == false
  • 69 vs. 80: likewise, fallback_mode == 0 isn't guaranteed here. If so, does the logic really represent the logic described for the three fallback modes?

I know these issues are independent of this PR but they need fixing anyway (if I'm not mistaken).

@@ -63,11 +69,21 @@ class BoincDb extends DbConn {
self::$instance = $instance;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh, GitHub doesn't let me comment outside of change contexts 🤦‍♂️ ...

The error_log two lines above should distinguish between <replica_db_host> and <replica2_db_host> or use <replica[2]_db_host> instead (if that's not too confusing).

@AenBleidd
Copy link
Member

@davidpanderson, could you please take a look at this PR?

@brevilo
Copy link
Contributor

brevilo commented Jul 18, 2024

Also, thanks to @RichardHaselgrove, we should make sure the docs get updated.

@davidpanderson while at it, please note that that doc contains a lot of syntax errors, presumably by a flawed trac -> markdown conversion. Just search for \< in it to see what I mean.

Thanks

@davidpanderson
Copy link
Contributor

The logic in get_aux() is too complex.
I'd prefer to clean it up (and handle replica2 in the process)

Is $fallback_mode needed? I can't find any place that uses it.
It would be simpler if we assume that

  • each replica has its own user/host/passwd in config.xml
  • if connection to replica 1 or 2 fails, use main DB

@bema-aei
Copy link
Contributor Author

bema-aei commented Jul 18, 2024

The fallback_mode was introduced by Christian in 2015, see 3481806 No idea (anymore) what it was used for, or in which projects, or which projects would break now if it was removed.

@davidpanderson
Copy link
Contributor

I added support for multiple readonly replicas in #5695,
using the same <replica2_db_host> config syntax.
This PR also greatly simplifies the get_aux() logic.
Please see if it works for you.

@brevilo
Copy link
Contributor

brevilo commented Jul 19, 2024

You mean #5697

@bema-aei bema-aei force-pushed the update_get_aux_for_replica2 branch from 8285a9d to 255aa04 Compare July 31, 2024 13:16
@bema-aei bema-aei force-pushed the update_get_aux_for_replica2 branch from 255aa04 to 7410470 Compare July 31, 2024 13:32
@bema-aei
Copy link
Contributor Author

bema-aei commented Jul 31, 2024

I just pushed a new version of the branch that takes David's commits from #5697 and applies a few changes:

  • Hiding a change that affects the whole web code ("use mysqli everywhere") in another commit with very limited scope and not even mentioning it in the commit message is just asking for trouble.. So I split that commit in two (leaving the author intact).
  • I added commit d1f99043877552ea48ac2912dc4ec9ac9a1efdddI mentioned above that is required to make this work for us. It allows to address multiple DB hosts in sequence.
  • I also changed the debug output to suit my needs, then reverted it. You can re-add it by reverting the 'Revert' commit if needed.

@davidpanderson
Copy link
Contributor

What's the use case? i.e. why would you need to access one replica, then another?

@brevilo
Copy link
Contributor

brevilo commented Aug 1, 2024

Different parts of the system use different replicas to spread the load and we're also showing the replication lag (of both replicas) on the SSP such that people can judge whether all data they see is up to date.

@bema-aei
Copy link
Contributor Author

bema-aei commented Aug 1, 2024

Actually the server status page where we show the status of all the replicas is the only case where we need to access all of these on the same page. With a bit of internal caching it should be possible to make sure there's only one connection open at a time, but I didn't think about that initially.

@AenBleidd
Copy link
Member

Closing this since #5697 was already merged to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants