TLS support, connection options & some refactoring

jaegertracing / jaeger-clickhouse

Jaeger ClickHouse storage plugin implementation

Apache License 2.0

247 stars 51 forks source link

TLS support, connection options & some refactoring #22

Closed EinKrebs closed 3 years ago

EinKrebs commented 3 years ago

Added database name, username and password options for connection to database;
abled connection using TLS;
did some code refactoring;
changed README.

Can you please give me code review and some ideas about README content?

Resolves #18

EinKrebs commented 3 years ago

Also:

Fixed span finding issue(check commit message for details);
Changed run options to make it more universal.

pavolloffay commented 3 years ago

@EinKrebs nit: generally try to keep changes in the PR minimal and rather split the PR into smaller PRs when doing multiple changes that do not belong together.

pavolloffay commented 3 years ago

@EinKrebs PR needs to be rebased

EinKrebs commented 3 years ago

@pavolloffay done.

pavolloffay commented 3 years ago

It seems that there are some issue

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

pavolloffay commented 3 years ago

I am not sure what is the issue but I have also noticed that you are submitting the PR from your main branch. I would suggest using feature branches and keep main in sync with the upstream main branch.

EinKrebs commented 3 years ago

Thank you for advice, it's really much easier to work like this.

EinKrebs commented 3 years ago

I don't know why branch can't be rebased, when I try to rebase it my git does nothing

EinKrebs commented 3 years ago

I made better version of this PR.