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

Add go mod #935

Merged
merged 14 commits into from
Jun 24, 2021
Merged

Add go mod #935

merged 14 commits into from
Jun 24, 2021

Conversation

timvaillancourt
Copy link
Collaborator

Description

This is a merge PR of the user-submitted PR: #932

Unfortunately this step is required as our gh-ost testing won't run on external forks

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.

cc @natewernimont-wk / @shlomi-noach

@timvaillancourt timvaillancourt mentioned this pull request Mar 3, 2021
2 tasks
Copy link

@tomdeering-wf tomdeering-wf left a comment

Choose a reason for hiding this comment

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

Thanks for being willing to accept this change! Now we'll be able to build gh-ost with Go 1.16 without having to revert go build back to GO111MODULE=auto (the default in Go 1.15).

Copy link
Contributor

@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.

Shoot for the stars

@natewernimont-wk natewernimont-wk mentioned this pull request Mar 5, 2021
2 tasks
@timvaillancourt timvaillancourt temporarily deployed to production/mysql_role=ghost_testing March 8, 2021 21:01 Inactive
@timvaillancourt timvaillancourt temporarily deployed to production/mysql_role=ghost_testing March 10, 2021 12:22 Inactive
@timvaillancourt timvaillancourt had a problem deploying to production/mysql_role=ghost_testing March 10, 2021 12:22 Failure
@timvaillancourt timvaillancourt temporarily deployed to production/mysql_role=ghost_testing March 10, 2021 12:22 Inactive
@timvaillancourt
Copy link
Collaborator Author

👋 @natewernimont-wk this ran into automated testing failures with the following output:

2021-03-10 04:24:24 FATAL Unable to read osc chunk size: strconv.ParseInt: parsing "": invalid syntax

A rollback to master was able to resolve this. Whatever the issue it is strange it didn't get caught in CI 🤔

I haven't had time to dig into "why", but the command that triggered the problem was:

gh-ost --conf=/etc/mysql/gh-ost.cnf --test-on-replica --database=REDACTED --table=REDACTED \
--verbose --alter=engine=innodb --execute --initially-drop-ghost-table --initially-drop-old-table \
--max-load=Threads_running=30 --critical-load=Threads_running=1000 --critical-load-interval-millis=3000 \
--switch-to-rbr --chunk-size=2500 --cut-over=default --timestamp-old-table --exact-rowcount \
--concurrent-rowcount --serve-socket-file=/tmp/gh-ost.test.sock --panic-flag-file=/tmp/ghost-test-panic.flag \
--hooks-path=REDACTED --hooks-hint=@root --ssl --ssl-allow-insecure --verbose

@timvaillancourt
Copy link
Collaborator Author

timvaillancourt commented May 28, 2021

@shlomi-noach this is failing on an error I could use some context around:

 go/logic/inspect.go:619:20: m.GetUint64 undefined (type sqlutils.RowMap has no field or method GetUint64)

It appears that function doesn't exist in github.com/outbrain/golib/sqlutils and I can't find a point in the history where it did exist. It does exist in vendor dir on master though 🤔

That lead me to wondering if a patch was committed to the vendor/ dir of this repo but the git history doesn't support that either

I'm curious if you had any ideas how this may have happened. I suppose the options are to add it to github.com/outbrain/golib or make a copy of that logic as a utility-func in go/ in this repo. I think I'm leaning towards the latter as the golib repo doesn't seem very current

@shlomi-noach
Copy link
Contributor

@timvaillancourt right. GetUint64 was added downstream, directly into vendor/ path, in openark@31069ae4

What I'm suggesting:

  • I have this better maintained repo: https://github.com/openark/golib
  • I'll add GetUint64 functionality in that repo
  • you will add replace github.com/outbrain/golib => github.com/openark/golib in go.mod

I'm just trying to think whether there's been more changes local to golib under vendor. I'll either git blame later on, or just confirm the code builds properly with openark/golib.

@shlomi-noach
Copy link
Contributor

shlomi-noach commented May 31, 2021

Related: outbrain-inc/golib#10 openark/golib#11

@shlomi-noach
Copy link
Contributor

shlomi-noach commented May 31, 2021

openark/golib#11 is merged, and gh-ost builds well with it.

@timvaillancourt either that replace directive, or just search&replace github.com/outbrain/golib with github.com/openark/golib throughout the code at your discretion.

@timvaillancourt
Copy link
Collaborator Author

openark/golib#11 is merged, and gh-ost builds well with it.

@shlomi-noach thanks for the PR and background ❤️! I'll look out for this if this PR runs into any other problems 👍

@timvaillancourt timvaillancourt temporarily deployed to production/mysql_cluster=mysql1&mysql_role=ghost_testing June 21, 2021 16:50 Inactive
@timvaillancourt timvaillancourt temporarily deployed to production/mysql_cluster=mysql1&mysql_role=ghost_testing June 21, 2021 16:50 Inactive
@timvaillancourt timvaillancourt deployed to production/mysql_role=ghost_testing&mysql_cluster=repositories June 21, 2021 18:20 Active
@timvaillancourt timvaillancourt had a problem deploying to production/mysql_role=ghost_testing&mysql_cluster=ballast June 21, 2021 18:20 Failure
@timvaillancourt timvaillancourt deployed to production/mysql_role=ghost_testing&mysql_cluster=collab June 21, 2021 18:20 Active
@timvaillancourt timvaillancourt deployed to production/mysql_role=ghost_testing&mysql_cluster=billing June 21, 2021 18:21 Active
@timvaillancourt
Copy link
Collaborator Author

timvaillancourt commented Jun 21, 2021

This is passing real-world testing as of commit: 71d27d0

If anyone has time to test this on more environments that would be much appreciated! 🙇

cc @natewernimont-wk / @tomdeering-wf / @shlomi-noach

@timvaillancourt timvaillancourt temporarily deployed to production/mysql_cluster=mysql1&mysql_role=ghost_testing June 23, 2021 14:08 Inactive
@timvaillancourt timvaillancourt had a problem deploying to production/mysql_role=ghost_testing&mysql_cluster=repositories June 23, 2021 14:08 Failure
@timvaillancourt timvaillancourt had a problem deploying to production/mysql_role=ghost_testing&mysql_cluster=ballast June 23, 2021 14:08 Failure
@timvaillancourt timvaillancourt had a problem deploying to production/mysql_role=ghost_testing&mysql_cluster=collab June 23, 2021 14:08 Failure
@timvaillancourt timvaillancourt had a problem deploying to production/mysql_role=ghost_testing&mysql_cluster=billing June 23, 2021 14:08 Failure
@timvaillancourt timvaillancourt deployed to production/mysql_cluster=collab&mysql_role=ghost_testing June 23, 2021 14:10 Active
@timvaillancourt timvaillancourt deployed to production/mysql_cluster=billing&mysql_role=ghost_testing June 23, 2021 14:10 Active
@timvaillancourt timvaillancourt deployed to production/mysql_cluster=mysql1&mysql_role=ghost_testing June 23, 2021 14:10 Active
@timvaillancourt timvaillancourt deployed to production/mysql_cluster=repositories&mysql_role=ghost_testing June 23, 2021 14:10 Active
@timvaillancourt
Copy link
Collaborator Author

This has passed several-100 tests on real tables, so I'll merge this 👍

@timvaillancourt timvaillancourt merged commit 47d49c6 into master Jun 24, 2021
@timvaillancourt timvaillancourt deleted the add-go-mod branch June 24, 2021 18:19
dm-2 added a commit that referenced this pull request Jan 6, 2022
This reverts commit 47d49c6.
RainbowDashy pushed a commit to bytebase/gh-ost that referenced this pull request Mar 10, 2022
* Add a go.mod file

* run go mod vendor again

* Move to a well-supported ini file reader

* Remove GO111MODULE=off

* Use go 1.16

* Rename github.com/outbrain/golib -> github.com/openark/golib

* Remove *.go-e files

* Fix for `strconv.ParseInt: parsing "": invalid syntax` error

* Add test for '[osc]' section

Co-authored-by: Nate Wernimont <[email protected]>
@d-bytebase
Copy link

Thanks for providing go mod for gh-ost! When are we going to have 1.1.5 release for the change?

@dm-2
Copy link
Contributor

dm-2 commented Mar 14, 2022

Hi @d-bytebase, we're currently investigating a data corruption bug that we believe may have been introduced by an updated dependency in this PR - once we've identified the cause and fixed it, the plan is to put out a new release based on the master branch (1.1.3 and 1.1.4 are based on 1.1.2).

@dm-2 dm-2 mentioned this pull request Jul 7, 2022
@dm-2 dm-2 modified the milestones: v1.1.5, v1.1.6 Jul 7, 2022
@chenrui333
Copy link

Looks like this change was never been released? Can we cut a release for this PR? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants