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

Added MySQL backend to /web/status. #216

Merged
merged 3 commits into from Jun 26, 2017
Merged

Conversation

maurosr
Copy link
Contributor

@maurosr maurosr commented Jun 26, 2017

In order to not have to manually check which mysql backend (host:port) orchestrator is configured to talk to but rather see it on the GUI under /web/status.
my_sql_backend

Copy link
Collaborator

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

This needs to adapt to support for sqlite, in which case there is no OrchestratorMySQLHost nor OrchestratorMySQLPort.
I'd like to please properly visualize the case where config.Config.IsSQLite

Regardless, you've created the columns NULL-able -- how does the app react when the values are NULL?
I'd advise NOT NULL columns, as the golang handling of NULL values is so dirty. An empty value for hostname is a great hint for "no value".

go/db/db.go Outdated
`
ALTER TABLE node_health
ADD COLUMN mysql_backend_hostname varchar(128) CHARACTER SET ascii,
ADD COLUMN mysql_backend_port smallint UNSIGNED
Copy link
Collaborator

Choose a reason for hiding this comment

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

columns are NULL-able

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of using NOT NULL DEFAULT "" and NOT NULL DEFAULT 0 respectively?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was my suggestion in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I will then test this and also add support for sqlite

AppVersion: m.GetString("app_version"),
FirstSeenActive: m.GetString("first_seen_active"),
LastSeenActive: m.GetString("last_seen_active"),
MySQLBackendHostname: m.GetString("mysql_backend_hostname"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if column was NULL?

FirstSeenActive: m.GetString("first_seen_active"),
LastSeenActive: m.GetString("last_seen_active"),
MySQLBackendHostname: m.GetString("mysql_backend_hostname"),
MySQLBackendPort: m.GetInt("mysql_backend_port"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Copy link
Collaborator

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

see comments inline

db_backend = config.Config.SQLite3DataFile
} else {
db_backend = config.Config.MySQLOrchestratorHost + ":" +
fmt.Sprintf("%d", config.Config.MySQLOrchestratorPort)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick, but the golang way, since you've already chosen fmt, is:

db_backend = fmt.Sprintf("%s:%d", config.Config.MySQLOrchestratorHost, config.Config.MySQLOrchestratorPort)

while at it, please change db_backend to dbBackend. golang's naming conventions use camelBack

@shlomi-noach shlomi-noach merged commit 753c4fe into openark:master Jun 26, 2017
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.

None yet

2 participants