search

vert-x3 / vertx-jdbc-client

JDBC support for Vert.x
Apache License 2.0
126 stars 90 forks source link

Make data source name configurable through JDBCPool #287

Closed mnylen closed 2 years ago

mnylen commented 2 years ago

Motivation:

With the old JDBC Client API it was possible to configure the data source name, but this option is not available when using JDBCPool that wraps the old JDBCClient into the new SQL Client API.

Instead, a random UUID is always used as the data source name. This causes a lot of problems with metrics such as:

a) It's impossible to know which data source the random UUID maps to. This can be a problem with projects using multiple data sources to different databases.

b) The data source names change on each deployment, which makes it impossible to follow trends over time.

c) Can add to the operating costs when there are a huge amount of servers when using e.g. CloudWatch which is billed per metric.

Conformance:

Your commits should be signed and you should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

vietj commented 2 years ago

can you add a test for this ?

mnylen commented 2 years ago

@vietj I sure can! I'm wondering whether there's any way for me to access the metrics from the tests. Do you have any pointers?

vietj commented 2 years ago

one test in CI is not passing with Java 17, can you try running tests (ClickHouseOldAPITest) with Java 17 and your branch ?

mnylen commented 2 years ago

@vietj Works for me with openjdk-17.0.1 and openjdk-11.0.12.

However, I couldn't run the tests exactly as is in this branch, because the clickhouse image used is not compatible with ARM architecture (running M1 Mac here) and test containers itself needed to be upgraded to properly work with M1 Macs (was complaining it couldn't find docker environment before upgrading to 1.15.3 and forcing jna:5.8.0).

After upgrading test containers to 1.15.3 and jna to 5.8.0, while still using the old yandex/clickhouse-server image, I was able to replicate the timeout issue. But that was due to the clickhouse server failing to start:

runtime: failed to create new OS thread (have 2 already; errno=22)
fatal error: newosproc

runtime stack:
runtime.throw(0x4cb21f, 0x9)
    /usr/local/go/src/runtime/panic.go:566 +0x95
runtime.newosproc(0xc420026000, 0xc420035fc0)
    /usr/local/go/src/runtime/os_linux.go:160 +0x194
runtime.newm(0x4d6db8, 0x0)
    /usr/local/go/src/runtime/proc.go:1572 +0x132
runtime.main.func1()
    /usr/local/go/src/runtime/proc.go:126 +0x36
runtime.systemstack(0x53ae00)
    /usr/local/go/src/runtime/asm_amd64.s:298 +0x79
runtime.mstart()
    /usr/local/go/src/runtime/proc.go:1079

goroutine 1 [running]:
runtime.systemstack_switch()
    /usr/local/go/src/runtime/asm_amd64.s:252 fp=0xc420020768 sp=0xc420020760
runtime.main()
    /usr/local/go/src/runtime/proc.go:127 +0x6c fp=0xc4200207c0 sp=0xc420020768
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:2086 +0x1 fp=0xc4200207c8 sp=0xc4200207c0

Changing the clickhouse-server image to clickhouse/clickhouse-server:22.8.1.2097-alpine that has both arm64 and x86 versions resolved the issue and no timeouts occurred.

I don't see how my changes could lead to a timeout in that test, so I'm suspecting it might be something with the clickhouse container failing to start in this particular CI build. Would be helpful to be able to see logs from the container. Any suggestions on how to debug this further? Would you mind re-running the build?

mnylen commented 2 years ago

Added a commit to print clickhouse container logs after the test. If you could re-run the workflow as I don't seem to have the rights to do that.

mnylen commented 2 years ago

@vietj you can actually see that this test has failed also on Java 8 during scheduled job just recently: https://github.com/vert-x3/vertx-jdbc-client/runs/7892106569?check_suite_focus=true So I don’t think this is related to my changes.

mnylen commented 2 years ago

Made another PR to make the tests runnable on M1 Macs. https://github.com/vert-x3/vertx-jdbc-client/pull/288

mnylen commented 2 years ago

Yeah, so this passed now with the debug logging (nothing really changed). I think the clickhouse container startup is somehow flaky. Can we run this in loop until it fails?

vietj commented 2 years ago

right now it is green

vietj commented 2 years ago

you can fork this in your own organisation and try, the github action is implemented to work on user's clones

On Mon, Aug 22, 2022 at 8:31 AM Mikko Nylén @.***> wrote:

Yeah, so this passed now with the debug logging (nothing really changed). I think the clickhouse container startup is somehow flaky. Can we run this in loop until it fails?

— Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-jdbc-client/pull/287#issuecomment-1221905708, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCXJ4IX4SGPISN55GM3V2MNDBANCNFSM5655NMCQ . You are receiving this because you were mentioned.Message ID: @.***>