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

Use connection pool for redis dictionary #33804

Merged
merged 3 commits into from
Jan 21, 2022

Conversation

CurtizJ
Copy link
Member

@CurtizJ CurtizJ commented Jan 20, 2022

Changelog category (leave one):

  • Bug Fix (user-visible misbehaviour in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix usage of external dictionaries with Redis source and large number of keys.

Fixes #27784.

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Jan 20, 2022
@CurtizJ CurtizJ added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Jan 20, 2022
@CurtizJ
Copy link
Member Author

CurtizJ commented Jan 20, 2022

BTW, performance of redis dictionaries is still questionable. Maybe we need to use another client library for redis. But ok for now.

@kitaisreal kitaisreal self-assigned this Jan 20, 2022
src/Dictionaries/RedisDictionarySource.cpp Outdated Show resolved Hide resolved
src/Dictionaries/RedisDictionarySource.cpp Outdated Show resolved Hide resolved
String RedisDictionarySource::toString() const
{
return "Redis: " + host + ':' + DB::toString(port);
}

auto RedisDictionarySource::getConnection() const -> ConnectionPtr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider to use ConnectionPtr as return type, without this auto.

Copy link
Member Author

@CurtizJ CurtizJ Jan 20, 2022

Choose a reason for hiding this comment

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

This construction helps to avoid extra qualification for ConnectionPtr. Otherwise we will have to write

RedisDictionarySource::ConnectionPtr RedisDictionarySource::getConnection() const

But I'm ok to replace this.

src/Dictionaries/RedisDictionarySource.cpp Outdated Show resolved Hide resolved
@kssenii kssenii removed their assignment Jan 20, 2022
@kitaisreal
Copy link
Collaborator

Not sure maybe it is better to backport only crash fix, without adding pool.

@CurtizJ
Copy link
Member Author

CurtizJ commented Jan 20, 2022

BTW, performance of redis dictionaries is still questionable.

More precisely, for direct dictionary it's about 300-400K rows for SELECT * FROM dict and about 60-80K rows for dictGet with complex key dictionary.

@kitaisreal
Copy link
Collaborator

@CurtizJ lets merge it that way, later we can investigate how performance can be improved.

@kitaisreal kitaisreal merged commit 548a7bc into ClickHouse:master Jan 21, 2022
@CurtizJ CurtizJ removed the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClickHouse is stuck when using a dictionary with Redis as the data source
4 participants