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

Default connection timeout is way too low (50ms). #47

Closed
solf opened this Issue Nov 3, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@solf

solf commented Nov 3, 2016

ClickHouseConnectionSettings.CONNECTION_TIMEOUT("connection_timeout", 50)

I traced this into Apache HTTP client being used -- this is milliseconds value even though it is specified as int.

With this default anything that is not very local will fail (in my case -- trying to connect to database in EU DC from Canada.

@serebrserg

This comment has been minimized.

Show comment
Hide comment
@serebrserg

serebrserg Nov 5, 2016

Contributor

Thanks for the report.
I think there is no value for this setting that is correct in all cases, so it doesn't matter which exact value it is. What matters is how it can be configured. If you connect from application code, you can override the value with ClickHouseProperties or with Properties where you can pass the value as textual property. If you connect with some application where you can't manage the code and use compiled jar with dependencies, you can recompile with the value that fits your needs. That isn't convenient, but I can't propose anything else for now.
I can't imagine all the use cases, so it would be nice if you described your case and proposed what could be changed to make configuration more convenient.

Contributor

serebrserg commented Nov 5, 2016

Thanks for the report.
I think there is no value for this setting that is correct in all cases, so it doesn't matter which exact value it is. What matters is how it can be configured. If you connect from application code, you can override the value with ClickHouseProperties or with Properties where you can pass the value as textual property. If you connect with some application where you can't manage the code and use compiled jar with dependencies, you can recompile with the value that fits your needs. That isn't convenient, but I can't propose anything else for now.
I can't imagine all the use cases, so it would be nice if you described your case and proposed what could be changed to make configuration more convenient.

@solf

This comment has been minimized.

Show comment
Hide comment
@solf

solf Nov 5, 2016

While I agree that there is probably no value that is 'correct' in all cases, I don't agree that 50ms is reasonable default. This is for opening TCPIP connection which I assume doesn't happen all that often (I assume that the previously opened connection is kept alive). What possible reason is there for making it fail unless you're very local to your database?

If you don't want to pick a number, a reasonable default would be to leave it unset -- so it defaults to whatever Java/OS default value is.

If you're extremely timing-conscious on your client side (you'd rather fail than wait for more than XX ms), you still have an option of setting connection property to whichever value you prefer.

My specific usecase right now was connecting to remote clickhouse w/ SquirrelSQL. I'm fairly sure (tho i haven't tried) that it is possible to set the connection properties there somewhere, so I certainly could've used connection_timeout workaround.

But that kind of isn't the point :) The point is that from the user point of view driver fails unexpectedly and it is very un-clear why & how to fix it. It works locally, but when you try to connect to remote machine it suddenly fails.

If you still think that having very short default connection_timeout is a good idea, at least please consider inserting information about connection_timeout setting into the error message. But personally I think 50ms default is unreasonable and needs to be higher (or not set at all as mentioned above).

solf commented Nov 5, 2016

While I agree that there is probably no value that is 'correct' in all cases, I don't agree that 50ms is reasonable default. This is for opening TCPIP connection which I assume doesn't happen all that often (I assume that the previously opened connection is kept alive). What possible reason is there for making it fail unless you're very local to your database?

If you don't want to pick a number, a reasonable default would be to leave it unset -- so it defaults to whatever Java/OS default value is.

If you're extremely timing-conscious on your client side (you'd rather fail than wait for more than XX ms), you still have an option of setting connection property to whichever value you prefer.

My specific usecase right now was connecting to remote clickhouse w/ SquirrelSQL. I'm fairly sure (tho i haven't tried) that it is possible to set the connection properties there somewhere, so I certainly could've used connection_timeout workaround.

But that kind of isn't the point :) The point is that from the user point of view driver fails unexpectedly and it is very un-clear why & how to fix it. It works locally, but when you try to connect to remote machine it suddenly fails.

If you still think that having very short default connection_timeout is a good idea, at least please consider inserting information about connection_timeout setting into the error message. But personally I think 50ms default is unreasonable and needs to be higher (or not set at all as mentioned above).

@serebrserg

This comment has been minimized.

Show comment
Hide comment
@serebrserg

serebrserg Nov 5, 2016

Contributor

I agree that it is unacceptable to make SQL IDE and other products users waste time chasing this timeout error. It must work with default value in all reasonable cases (I think 5 seconds is more than enough).
Although it is easy to override defaults from applications code, some applications may rely on defaults, so the changes in defaults must be announced.
I'll consider updating the timeout versus some other option that I don't have yet.

Contributor

serebrserg commented Nov 5, 2016

I agree that it is unacceptable to make SQL IDE and other products users waste time chasing this timeout error. It must work with default value in all reasonable cases (I think 5 seconds is more than enough).
Although it is easy to override defaults from applications code, some applications may rely on defaults, so the changes in defaults must be announced.
I'll consider updating the timeout versus some other option that I don't have yet.

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