-
Notifications
You must be signed in to change notification settings - Fork 562
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
Handle double pointers for Nullable columns when batch inserting #774
Handle double pointers for Nullable columns when batch inserting #774
Conversation
97af63a
to
cd0ac6d
Compare
<listen_host>::</listen_host> | ||
<listen_host>0.0.0.0</listen_host> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<listen_host>::</listen_host>
already means to listen on all hosts. No need to specify 0.0.0.0
as well (and is double specified).
Moved port definitions closer to listen host for easier spotting.
WaitingFor: wait.ForAll( | ||
wait.ForLog("Ready for connections").WithStartupTimeout(time.Second*time.Duration(120)), | ||
wait.ForSQL("9000/tcp", "clickhouse", func(port nat.Port) string { | ||
return fmt.Sprintf("clickhouse://default:[email protected]:%s", port.Port()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed localhost
to 127.0.0.1
explicitly, because on IPv6 enabled systems this might be pointing to the IPv6 loopback address ::1
:
$ cat /etc/hosts
127.0.0.1 localhost
127.0.1.1 housepaper
::1 localhost ip6-localhost ip6-loopback
ff02::1 ip6-allnodes
ff02::2 ip6-allrouters
In that scenario, startup of the container might timeout and ClickHouse only shows very strange error messages.
When debugging, it can be seen that the golang net.Dialer
prefers the IPv6 address above the IPv4 one, though with the incorrect port. Using 127.0.0.1
makes it explicit IPv4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think it's fine for now to use IPv4 explicitly.
I'll see with the Testcontainers project if its possible to create an issue / PR to solve this, specifying whether you want the IPv4 or IPv6 mapped port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolves #777
When batch inserting
Nullable
type, check if the type is a double pointer and if so, 'unwrap' to just a single pointer.Made a small proposal to force IPv4 usage when using Docker for tests, as at least locally I'm running into irregular behaviour. Happy to revert if we don't think this is a good idea...