Skip to content

Conversation

@SocalNick
Copy link
Contributor

@SocalNick SocalNick commented Sep 17, 2021

Related issue: #1026

Description

Status is currently only sent to stdout. We'd like the status to also be sent to the migrationContext logger.

In case this PR introduced Go code changes:

  • contributed code is using same conventions as original code
  • script/cibuild returns with no formatting errors, build errors or unit test errors.

Status is currently only sent to stdout. We'd like the status to also be
sent to the migrationContext logger.
@SocalNick
Copy link
Contributor Author

cc @abeyum

@SocalNick
Copy link
Contributor Author

Any chance someone could take a look and consider this PR? Thanks!

@timvaillancourt
Copy link
Collaborator

@SocalNick could you provide a before/after example of logging from this PR? Feel free to redact any sensitive info

Assuming this changes the logging output is there a way this change could be opt-in for backwards compatibility?

@timvaillancourt timvaillancourt requested review from a user and rashiq January 26, 2022 22:48
@SocalNick
Copy link
Contributor Author

Hey @timvaillancourt - apologies, I missed your comment in January.

This was actually a miss from when we contributed to logging interface in 2019/20.

Before this change, the lines below with msg="Copy would only be sent to stdout. This change ensures the migration status lines are also printed to the configured migration context logger.

time="2022-03-15T11:46:51Z" level=info msg="Exact number of rows via COUNT: 200" migration=b6244fb7-7b39-48f4-a54c-d6cd83bc2d18

time="2022-03-15T11:46:51Z" level=info msg="First throttle metrics collected" migration=b6244fb7-7b39-48f4-a54c-d6cd83bc2d18

time="2022-03-15T11:46:51Z" level=info msg="Copy: 0/200 0.0%!;(MISSING) Applied: 0; Backlog: 0/1000; Time: 0s(total), 0s(copy); streamer: mysql-bin-changelog.000031:16847320; Lag: 0.02s, State: migrating; ETA: N/A" migration=b6244fb7-7b39-48f4-a54c-d6cd83bc2d18

time="2022-03-15T11:46:52Z" level=info msg="Copy: 0/200 0.0%!;(MISSING) Applied: 0; Backlog: 0/1000; Time: 1s(total), 1s(copy); streamer: mysql-bin-changelog.000031:16848554; Lag: 0.01s, State: migrating; ETA: N/A" migration=b6244fb7-7b39-48f4-a54c-d6cd83bc2d18

time="2022-03-15T11:46:52Z" level=info msg="Row copy complete" migration=b6244fb7-7b39-48f4-a54c-d6cd83bc2d18

time="2022-03-15T11:46:52Z" level=info msg="Copy: 200/200 100.0%!;(MISSING) Applied: 0; Backlog: 0/1000; Time: 1s(total), 1s(copy); streamer: mysql-bin-changelog.000031:16949173; Lag: 0.01s, State: migrating; ETA: due" migration=b6244fb7-7b39-48f4-a54c-d6cd83bc2d18

FWIW, there's a much simpler way to make this change. In our fork, we added this.migrationContext.Log.Infof(status) immediately after the line that prints to stdout.

@timvaillancourt
Copy link
Collaborator

@SocalNick makes sense, thanks!

I think this makes sense to merge once this PR is rebased. Currently it has conflicts that must be resolved

@SocalNick
Copy link
Contributor Author

@timvaillancourt we've archived our fork so I reopened as #1193

I also made a very simple alternative in #1194

@SocalNick SocalNick closed this Oct 27, 2022
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.

2 participants