Fixes #4832: Add migration script to change the ids in database #319
Conversation
| # Upgrade script for Rudder database | ||
| ##################################################################################### | ||
| # This script upgrade existing Rudder database installation, to change the format of | ||
| # the id used, from int to long |
There was a problem hiding this comment.
Could you clarify what this script does exactly please? Basically an outline of the logic (renaming tables, migrating node by node, etc). This is a pretty in depth operation, it needs to be documented.
There was a problem hiding this comment.
Ah, and I thought "long" was "BigInt" ?
|
I have formulated a lot of comments here - sorry about the quantity, but since I know you are not that familiar with bash, I wanted to be clear and list all issues exhaustively to make it easier for you to fix them. Altogether, this looks like a great approach to solving the problem. In general, it does need a lot more explaining for the user though - running a migration script that fiddles with a database is not a comfortable situation, and would be made worse by a "black box" that "does stuff" without explaining what (and very hard to debug, fix, etc). |
|
Thank you for your feedback Jon ! This has been most helpful |
|
PR has been updated |
| echo "Upgrade script for Rudder database." | ||
| echo "" | ||
| echo "This script updates the format of all the id used, from Integer to Long" | ||
| echo "Please note that it will stop the Rudder web interface temporarily at the beginning ot the migration, for a few seconds" |
| # define the number of reports to migrate per node | ||
| # The allows values are number, or the special value "all" | ||
| # If it is all, then we say we migrate at least 1G report per node (SQL don't accept 'all' as a limit) | ||
| if [ z${NB_REPORT_REQUESTED} = "zall" ]; then |
There was a problem hiding this comment.
This should be quoted:
"z${NB_REPORT_REQUESTED}"
|
I've added some more (but less this time!) comments. Could you address them? Aside from these, this is looking pretty good! |
|
PR has been updated ! |
| RUDDER_UPGRADE_TOOLS=${RUDDER_SHARE}/upgrade-tools | ||
|
|
||
| # The int type in PostreSQL supports up to 2^32 entries, which is about 2000000000 | ||
| readonly MAX_REPORTS=2000000 |
There was a problem hiding this comment.
Er, there's definitely some digits missing here compared to the comment above!
And if it is 2^32, why not use the exact number? I see no need for approximations here.
…or_bigint Fixes #4832: Add migration script to change the ids in database
No description provided.