search

go-graphite / carbon-clickhouse

Graphite metrics receiver with ClickHouse as storage
MIT License
186 stars 48 forks source link

Get rid of old Clickhouse legacy? #29

Closed blind-oracle closed 5 years ago

blind-oracle commented 5 years ago

@lomik Do you think it's time to change the following:

  1. Convert table creation SQL to a new CH format?
  2. Remove the unneeded Date column from graphite_tree table? Recent CH allows to specify arbitrary primary key.
  3. Rename Timestamp to Version in series table? To avoid confusion.

I can provide patches if it's feasible.

lomik commented 5 years ago

Yes, it seems it is already possible to remove support of very old CH versions.

blind-oracle commented 5 years ago

@lomik Good, I've started the cleanup, will do a PR.

One more question: Deleted column. Is it used anywhere? carbon-clickhouse does not seem to write or update it and graphite-clickhouse only checks if it is = 0 in a number of queries.

Maybe we should drop it from both of them and from tables?

lomik commented 5 years ago

I think that Deleted was no longer needed after release of ALTER TABLE DELETE

blind-oracle commented 5 years ago

Ok, then we can drop it safely probably from both daemons.

blind-oracle commented 5 years ago

@lomik BTW If I'm reading CH docs correctly we need to have a DateTime column for GraphiteMergeTree table engine, but now we have uint32 for it with unix timestamp.

Although Clickhouse seem to work fine with it maybe we need to move to the documented structure, what do you think?

lomik commented 5 years ago

May be problem in documentation

Graphouse is using uint32 too https://github.com/yandex/graphouse/blob/master/doc/install.md

I know another problem with documentation https://github.com/yandex/ClickHouse/issues/1960

blind-oracle commented 5 years ago

@lomik It looks like that DateTime type is just a representation of uint32 column type (CH does not have a separate column type for Dates). So CH does not care for the data type, it just treats it as UNIX timestamp under the hood anyway.

So docs are right, they just don't state that uint's will work too.