github / gh-ost

GitHub's Online Schema-migration Tool for MySQL
MIT License
12.11k stars 1.23k forks source link

Print status to migration context logging after escaping % character #1374

Closed icyflame closed 5 months ago

icyflame commented 5 months ago

Description

The string status has the character % after the progress percentage number. When printed once again using the Infof function, this % is read as a directive for formatting and ends up showing MISSING in the output as shown below:

2024-01-31 12:49:54 INFO Copy: 0/0 100.0%!;(MISSING) Applied: 0; Backlog: 0/1000; Time: 0s(total), 0s(copy); streamer: mysql_bin.000002:79674; Lag: 0.00s, HeartbeatLag: 0.02s, State: migrating; ETA: due

Simply changing Infof to Info does not work here because this.migrationContext.Log is an object of the type DefaultLogger in this repository, which uses the library https://github.com/outbrain/golib/log. That library uses formatting even when the Infofunction is called: https://github.com/outbrain/golib/blob/2531e5dbcc71b6f8a4ccf1205c209ae89b7529fc/log/log.go#L191-L193

If this patch is not acceptible, then I can also just remove this line. The migration context logger prints the messages which are already printed to STDOUT once again to STDERR with the current time as a prefix. This change was introduced in the commit https://github.com/github/gh-ost/blob/515aa72d3d9b756e454b0168b4e57bc599b45e36/go/logic/migrator.go#L1039, introduced in the PR https://github.com/github/gh-ost/pull/1194.

A Pull Request should be associated with an Issue.

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs. If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues, and potentially we'll be able to point development in a particular direction.

Related issue: https://github.com/github/gh-ost/issues/1373

Further notes in https://github.com/github/gh-ost/blob/master/.github/CONTRIBUTING.md Thank you! We are open to PRs, but please understand if for technical reasons we are unable to accept each and any PR

In case this PR introduced Go code changes:

meiji163 commented 5 months ago

Thanks for chasing down the formatting bug~!

timvaillancourt commented 5 months ago

Awesome thanks! This reminds me we should move to a modern logger at some point