Skip to content

feat: Rework SQL connection check to application lifecylce#3857

Open
awildturtok wants to merge 9 commits intodevelopfrom
feature/sql-healthcheck-rework
Open

feat: Rework SQL connection check to application lifecylce#3857
awildturtok wants to merge 9 commits intodevelopfrom
feature/sql-healthcheck-rework

Conversation

@awildturtok
Copy link
Copy Markdown
Collaborator

No description provided.

@awildturtok awildturtok requested a review from thoniTUB as a code owner March 11, 2026 14:57
@awildturtok awildturtok force-pushed the feature/sql-healthcheck-rework branch from 96d9921 to 19b5de0 Compare March 11, 2026 15:02
log.info("SUCCESS connecting to {}", getJdbcConnectionUrl());
}
else {
log.error("FAILED connecting to {}. Connection did not become valid.", getJdbcConnectionUrl());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sind in der Url Credentials enthalten?

initializeDataSource();
}

public void initializeDataSource() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zum Beispiel so

Suggested change
public void initializeDataSource() {
public ManagedDataSource initializeDataSource() {

* Keys must match the name of existing {@link Dataset}s.
*/
private Map<String, @Valid DatabaseConfig> databaseConfigs;
private Map<DatasetId, @Valid DatabaseConnection> databaseConfigs;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funktioniert die Deserialisierung der Keys einfach so ohne Annotationen?

Comment on lines 64 to 67
public void close() {
closeDslContextWrapper();
//TODO do we even need to shutdown the connection?
super.close();
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Denke schon, wenn man ein Dataset löscht und dann wieder anlegt, sollte sich auch die Connection erneuern.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Das Ding ist halt, die Connection liegt nicht im Dataset sondern in der config.json

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deswegen sollte das nicht in der Config gehalten werden, sondern in einem separaten Objekt gemanaged werden. Bei Solr habe ich letztendlich die Dataset bezogenen Connections nicht über den Lifecycle gemanaged, weil sie nur bedingt an diesen gebunden sind.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Der Zweck des PRs ist ja aber die Connection an den Lifecylce zu binden und eben nicht mehr an das Dataset. Damit es dann auch beim hochfahren direkt gefangen wird etc. Ich räum es aber auf jeden Fall auf

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Das hat sich dann jetzt erledigt, oder die Connection wird nicht mehr hier gemanaged

getDialect().getJooqDialect(),
settings
);
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bitte diese Methode auch in eine ManagedDatasource packen

Comment on lines +87 to +90
// TODO this could be implemented as a plugin tbh
if(config.getSqlConnectorConfig() != null) {
config.getSqlConnectorConfig().initialize(environment);
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oder ein ConfiguredBundle, da kannst du checken ob SqlConnectorConfig gesetzt/enabled ist, Resources/Services registrieren für Injection, Healthchecks setzten...

@awildturtok awildturtok force-pushed the feature/sql-healthcheck-rework branch from 1c0f11d to 054aef3 Compare March 25, 2026 12:30
@awildturtok awildturtok changed the title Feature/sql healthcheck rework feat: Rework SQL connection check to application lifecylce Mar 25, 2026
@awildturtok awildturtok requested a review from thoniTUB March 25, 2026 15:02
Copy link
Copy Markdown
Collaborator

@thoniTUB thoniTUB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich bin mir nicht ganz sicher hat das mit der Clock etwas mit dem Lifecycle zu tun oder kannst du das trennen?

Comment on lines 64 to 67
public void close() {
closeDslContextWrapper();
//TODO do we even need to shutdown the connection?
super.close();
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Das hat sich dann jetzt erledigt, oder die Connection wird nicht mehr hier gemanaged


// join full stratification with connector table on all ID's from prerequisite query
SqlFunctionProvider functionProvider = tableContext.getConversionContext().getSqlDialect().getFunctionProvider();
SqlFunctionProvider functionProvider = tableContext.getConversionContext().getDialectBundle().getFunctionProvider();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey().du().hast().mir().mal().vom().LoD().erzählt() :D

Comment on lines +75 to +95
@SneakyThrows
public static void waitUntil(Supplier<Boolean> condition) {
Stopwatch stopwatch = Stopwatch.createStarted();
int done = 0;

while (stopwatch.elapsed(TimeUnit.SECONDS) < 10) {
Thread.sleep(2);
if (!condition.get()) {
continue;
}

//sample multiple times from the job queues to make sure we are done with everything and don't miss late arrivals
done++;
if (done > 5) {
return;
}
}

throw new IllegalStateException("Jobs did not finish within expected time.");
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich weiß die ist nur verschoben, aber kann ggf mit https://github.com/awaitility/awaitility abgebildet werden. Das sollte auch schon in der pom sein

@awildturtok
Copy link
Copy Markdown
Collaborator Author

Ich bin mir nicht ganz sicher hat das mit der Clock etwas mit dem Lifecycle zu tun oder kannst du das trennen?

Der Grund, dass das hier mit reingerutscht ist, ist dass ich die Komponenten die davor den DateNowSupplier gehalten habe aufgetrennt habe und den dann von wo anders mitbringen musste.

public interface DialectBundle {

SystemDateNowSupplier SYSTEM_DATE_NOW_SUPPLIER = new SystemDateNowSupplier();
//TODO add getDateNow and access it directly
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ist das noch zu tun?

import org.jooq.Field;

public interface SqlDialect {
//TODO unify with com.bakdata.conquery.models.config.Dialect
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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