Skip to content
This repository was archived by the owner on Feb 18, 2025. It is now read-only.

Conversation

@luisyonaldo
Copy link
Contributor

Solves issue: #1205

Description

This PR sets semi_sync_master_timeout as bigint to avoid out of range (overflow) errors.

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.

thank you, a couple changes requested

@shlomi-noach
Copy link
Collaborator

(you can see all CI jobs failed -- that's because of the sqlite3 missing hint)

luisyonaldo and others added 2 commits July 7, 2020 17:54
Co-authored-by: Shlomi Noach <[email protected]>
Co-authored-by: Shlomi Noach <[email protected]>
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.

we need to further change the matching go variable to int64 or uint64, correct? Let's do this in this PR, too

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.

I think last requested changes

@luisyonaldo
Copy link
Contributor Author

I think last requested changes

I didn't do uint64 because is not supported by sqlutils library, and all other cases that use bigint unsigned have int64 as counterpart in go

@shlomi-noach
Copy link
Collaborator

shlomi-noach commented Jul 8, 2020

there is no GetUint64

Whoops. But I think we must use it, right? Because the value reported is 2^64-1, correct?

@shlomi-noach
Copy link
Collaborator

I know why sqlutils dosn't have GetUint64 -- that's because golang's ParseInt doen't support parsing a uint64.

@shlomi-noach
Copy link
Collaborator

I wonder if we should literally first compare the string with 18446744073709551615

@luisyonaldo
Copy link
Contributor Author

I know why sqlutils dosn't have GetUint64 -- that's because golang's ParseInt doen't support parsing a uint64.

it does, ParseUint

@shlomi-noach
Copy link
Collaborator

it does, ParseUint

!!!

The name confused me, I assumed it returns a uint, not uint64. So inconsistent

@shlomi-noach
Copy link
Collaborator

See #1209

@luisyonaldo
Copy link
Contributor Author

luisyonaldo commented Jul 8, 2020

See #1209

Looks good, would be good if we can make #1104 work, so we can backport all this changes to their own repo.

Will wait for #1209 to be merged and then apply changes here

@shlomi-noach
Copy link
Collaborator

GetUint64 is now available

@shlomi-noach
Copy link
Collaborator

Looks good, would be good if we can make #1104 work

Yeah, I'm 💯 👍 on that. sqlutils has diverged a lot from the original codebase; iirc there's a bit of mess because I also made local changes to sqlutils.go in https://github.com/github/gh-ost

@shlomi-noach shlomi-noach merged commit d219844 into openark:master Jul 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants