bugfixes for sql matching stats - #3915
Conversation
7d52893 to
d679125
Compare
| loadNamespaces(); | ||
|
|
||
| loadMetaStorage(); | ||
|
|
There was a problem hiding this comment.
Wenn du das verschiebst, kann es gut sein, dass du die nachfolgenden Zeilen auch noch verschieben musst
There was a problem hiding this comment.
Muss mal schauen, ob es neustarts überlebt, aber sah bisher tatsächlich so aus. Hatte den branch schon ausführlich verwendet um matching stats zu testen
| connection.getConnection() | ||
| ); | ||
| } catch (Exception e) { | ||
| log.error("Failed to load namespaceStorage for {}", namespaceStorage.getDataset(), e); |
There was a problem hiding this comment.
Kannst du hier namespaceStorage.getDataset() aufrufen, wenn der Storage gefailed ist? Das sieht falsch aus
There was a problem hiding this comment.
Guter Punkt, danke
| } | ||
| catch (SQLException exception) { | ||
| log.error("FAILED connecting to {}", connection.getJdbcConnectionUrl(), exception); | ||
| throw exception; |
There was a problem hiding this comment.
Dadurch muss beim Startup die Datenbank schon verfügbar sein und kann nicht erst verzögert bereit sein. Ist für kubernetes okay, da der Pod dann neugestartet werden kann, aber du könntest auch die Exception mit der neuen Message wrappen als Cause un dann würde der Exception nicht doppelt geloggt werden (Ich gehe davon aus, dass die Exception dann nochmal geloggt wird )
There was a problem hiding this comment.
Für mich ist das schon Anforderung der Anwendung, dass SQL verfügbar ist, wenn es startet? Siehst du das anders? Mir gehts auch darum, dass ein falsches Passwort den Start abbricht, statt erst später beim Anfrage ausführen (also ggf. gar nicht) knallt
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| @Slf4j | ||
| @Data |
There was a problem hiding this comment.
Kannst du hier noch die @EqualsAndHashCode(callSuper=?) ergänzen, um die Compiler Warnings wegzumachen
| } | ||
| try { | ||
| someJob.get(30, TimeUnit.SECONDS); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Spezifischer: catch (TimeoutException e) ?
| if (!(concept instanceof TreeConcept)) { | ||
| continue; | ||
| } | ||
| jobsByConcept.put(concept.getId(), matchingStats.collectMatchingStatsForConcept((TreeConcept) concept, executorService, 5)); |
There was a problem hiding this comment.
| if (!(concept instanceof TreeConcept)) { | |
| continue; | |
| } | |
| jobsByConcept.put(concept.getId(), matchingStats.collectMatchingStatsForConcept((TreeConcept) concept, executorService, 5)); | |
| if (concept instanceof TreeConcept treeConcept) { | |
| jobsByConcept.put(concept.getId(), matchingStats.collectMatchingStatsForConcept(treeConcept, executorService, 5)); | |
| } |
| @Min(1) | ||
| @Builder.Default | ||
| private int matchingStatsWorkers = 5; |
There was a problem hiding this comment.
Wird glaube ich noch nicht genutzt, im backend/src/main/java/com/bakdata/conquery/mode/local/UpdateMatchingStatsSqlJob.java hast du noch die 5 als Magicnumber
There was a problem hiding this comment.
Doch, die wird verwendet und ist eine andere magic number. Aber ja kann die retries auch konfigurieren
| /** | ||
| * collect unique fields used/defined in the expressions. | ||
| */ | ||
| private static List<Field<?>> collectAllFields(List<CTCondition.ConceptConditions> conceptConditions) { |
There was a problem hiding this comment.
| /** | |
| * collect unique fields used/defined in the expressions. | |
| */ | |
| private static List<Field<?>> collectAllFields(List<CTCondition.ConceptConditions> conceptConditions) { | |
| /** | |
| * collect unique fields used/defined in the expressions. | |
| */ | |
| private static List<Field<?>> collectReferencedFields(List<CTCondition.ConceptConditions> conceptConditions) { |
|
|
||
| Stopwatch stopwatch = Stopwatch.createStarted(); | ||
| MatchingStats stats = new MatchingStats(); | ||
| stats.putEntry("sql", entry.getValue()); |
There was a problem hiding this comment.
Dass der Entry hier "sql" heißt, sollte das nicht aus einer Konstante kommen?
| DialectBundle dialectBundle, | ||
| DSLContext dslContext, | ||
| SqlExecutionService executionService, Clock clock | ||
| SqlExecutionService executionService, Clock clock, String defaultPrimaryColumn |
There was a problem hiding this comment.
| SqlExecutionService executionService, Clock clock, String defaultPrimaryColumn | |
| SqlExecutionService executionService, | |
| Clock clock, | |
| String defaultPrimaryColumn |
2 participants
No description provided.