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.

Sign up for GitHub

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

Jump to bottom

RONDB-205: Add options --mysqld-instrumentation and --extra-packages #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

RONDB-205: Add options --mysqld-instrumentation and --extra-packages #5

wants to merge 3 commits into from

Conversation

svenssonaxel
Copy link
Collaborator

No description provided.

olapiv
olapiv reviewed Jan 18, 2023
View reviewed changes
Copy link
Collaborator

@olapiv olapiv left a comment

Choose a reason for hiding this comment

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

Looks good. Just wondering whether the Docker image tag should be changed accordingly if we install the extra packages? I don't want to rebuild everything if I try out this feature.

olapiv
olapiv reviewed Jan 18, 2023
View reviewed changes
build_run_docker.sh Outdated Show resolved Hide resolved
@svenssonaxel
Copy link
Collaborator Author

Looks good. Just wondering whether the Docker image tag should be changed accordingly if we install the extra packages? I don't want to rebuild everything if I try out this feature.

Fair point. If the tag is overwritten and you clear the cache you'd have to rebuild, so it's probably better to make unique tags. I'll push a fix tomorrow.

@olapiv
Copy link
Collaborator

olapiv commented Jan 18, 2023

Just tested it and for me the sleep 25 for the API node did not suffice. It takes more like 120s with instrumentation. But I've exchanged the ndb_waiter for a Docker MySQLd healthcheck in the rest-api branch that I'm working on, which will work a lot more accurately anyways. The ndb_waiter returns STARTED if the cluster has started - that does not mean that the MySQL server is ready yet.

@svenssonaxel
Copy link
Collaborator Author

I've exchanged the ndb_waiter for a Docker MySQLd healthcheck in the rest-api branch that I'm working on

Perhaps it would make sense to wait with this PR until that is merged?

@olapiv
Copy link
Collaborator

olapiv commented Jan 18, 2023

Yeah maybe. Could still take a couple of weeks, but then we're sure this works without hacking the script.

@svenssonaxel
Copy link
Collaborator Author

Yeah maybe. Could still take a couple of weeks, but then we're sure this works without hacking the script.

I don't think we're in a hurry with this one.

@svenssonaxel
RONDB-205: Fixes
ee54619 
- Use only 4 hexadecimal digits to identify instrumentation command line.
- Use only 4 hexadecimal digits to identify extra packages.
- Let tag name depend on instrumentation command line and extra packages.
@svenssonaxel
Copy link
Collaborator Author

Docker image tag should be changed accordingly

Fixed in ee54619.

@olapiv
Copy link
Collaborator

olapiv commented Feb 22, 2023

Needs #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olapiv olapiv olapiv left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@svenssonaxel @olapiv