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

Set update_time on full executable dictionary update #4551

Merged
merged 1 commit into from
Mar 1, 2019

Conversation

temoon
Copy link
Contributor

@temoon temoon commented Mar 1, 2019

This fixes issue #4524.

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

For changelog. Remove if this is non-significant change.

Category (leave one):

  • Bug Fix

Short description (up to few sentences):
Set update_time on full executable dictionary update.

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Mar 1, 2019

The code (not yours) is terrible and should be rewritten.

std::string getUpdateFieldAndDate();
There is no comment about this method.
The name of the method is misleading.
It is not qualified as const despite its name and it changes update time by surprise.

char buffer[80];
struct tm * timeinfo;
timeinfo = localtime(&hr_time);
strftime(buffer, 80, "\"%Y-%m-%d %H:%M:%S\"", timeinfo);
std::string str_time(buffer);

Using obsolete C functions for time formatting. DateLUT can be used instead.

@alexey-milovidov alexey-milovidov self-requested a review March 1, 2019 16:09
@alexey-milovidov alexey-milovidov merged commit f74252c into ClickHouse:master Mar 1, 2019
@alexey-milovidov
Copy link
Member

The logic in code is also flawed. We cannot use client side time to compare it on server side.

alexey-milovidov added a commit that referenced this pull request Mar 1, 2019
@temoon
Copy link
Contributor Author

temoon commented Mar 1, 2019

Thanks for reply, Alexey.

I use of about 70 million rows sized external dictionary which has frequently updates (~5k entries per second), and full dump tooks few minutes :((
Even is this form of functionality is very helpful for me!

@temoon temoon deleted the temoon-iss-4524 branch March 1, 2019 16:45
@alexey-milovidov
Copy link
Member

Ok. I have rewritten it a little, but I doubt that it is covered by tests, so good luck.

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

Successfully merging this pull request may close these issues.

2 participants