Skip to content

Feature/sql healthcheck rework#3857

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

Feature/sql healthcheck rework#3857
awildturtok wants to merge 2 commits intodevelopfrom
feature/sql-healthcheck-rework

Conversation

@awildturtok
Copy link
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
Comment on lines +77 to +80
@Override
public void start() throws Exception {
initializeDataSource();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trenne bitte die Config von dem Managed, so dass initializeDataSource gekapselt ist und nur einmal pro Connection aufgerufen werden kann

Comment on lines +71 to +75
@JsonIgnore
private HikariDataSource dataSource;

@JsonIgnore
private HealthCheckRegistry healthCheckRegistry;
Copy link
Collaborator

Choose a reason for hiding this comment

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

State bitte in einem Service halten, nicht in der Config.

log.info("SUCCESS connecting to {}", getJdbcConnectionUrl());
}
else {
log.error("FAILED connecting to {}. Connection did not become valid.", getJdbcConnectionUrl());
Copy link
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?

IdColumnConfig idColumns = config.getIdColumns();
SqlConnectorConfig sqlConnectorConfig = config.getSqlConnectorConfig();
DatabaseConfig databaseConfig = sqlConnectorConfig.getDatabaseConfig(namespaceStorage.getDataset());
DatabaseConnection databaseConfig = sqlConnectorConfig.getDatabaseConfig(namespaceStorage.getDataset());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
DatabaseConnection databaseConfig = sqlConnectorConfig.getDatabaseConfig(namespaceStorage.getDataset());
DatabaseConnection databaseConnection = sqlConnectorConfig.getDatabaseConfig(namespaceStorage.getDataset());

initializeDataSource();
}

public void initializeDataSource() {
Copy link
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
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
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
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
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.

getDialect().getJooqDialect(),
settings
);
}
Copy link
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
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...

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