Bump clickhouse-go: v1.5.4 -> v2.3.0

jaegertracing / jaeger-clickhouse

Jaeger ClickHouse storage plugin implementation

Apache License 2.0

249 stars 51 forks source link

Bump clickhouse-go: v1.5.4 -> v2.3.0 #116

Closed chhetripradeep closed 2 years ago

chhetripradeep commented 2 years ago

Signed-off-by: Pradeep Chhetri pradeepchhetri4444@gmail.com

Which problem is this PR solving?

Resolves https://github.com/jaegertracing/jaeger-clickhouse/issues/113

Short description of the changes

This change will give good performance gain.

lodthe commented 2 years ago

Thanks for the suggested changes!

I have some thoughts on how we can save compatibility:

When we create a connector, we can trim the prefix if the given address starts with tcp://.
When we insert/select data, can we cast span duration to uint64 and leave the old type in the DB scheme? It seems logically OK for me that the duration of a span is a positive number... But I'm not very familiar with the specifics of Jaeger.

chhetripradeep commented 2 years ago

Hi @lodthe

Thank you so much for the review.

When we create a connector, we can trim the prefix if the given address starts with tcp://.

I was also thinking along the same lines. Great, I will make this change.

When we insert/select data, can we cast span duration to uint64 and leave the old type in the DB scheme.

I was trying to make it work like this but somehow wasn't able to get it working. I will spend more time to see if we can achieve it.

Best Regards.

chhetripradeep commented 2 years ago

Hi @lodthe

Thank you for the feedbacks earlier. Both of the concerns are now resolved and changes are now fully backward incompatible. Would love to have your review again.

chhetripradeep commented 2 years ago

Thank you for the feedback. Updated the PR as per your suggestions.

chhetripradeep commented 2 years ago

Hi @nickbp

Will be great to have your review. Thank you!

nickbp commented 2 years ago

Oh whoops didn't realize this was waiting on me. It looks great, also thanks for tidying up the imports and the CLose

nickbp commented 2 years ago

FYI I've cut a new 0.12.0 release including this change just now. It had been a few months since the last release and a few improvements had come in since then.