Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove CentralRegistry #3554

Merged
merged 1 commit into from
Oct 8, 2024
Merged

remove CentralRegistry #3554

merged 1 commit into from
Oct 8, 2024

Conversation

thoniTUB
Copy link
Collaborator

No description provided.

awildturtok
awildturtok requested changes Sep 16, 2024
View reviewed changes
backend/src/main/java/com/bakdata/conquery/io/jackson/serializer/NsIdRefCollection.java
@@ -1,24 +0,0 @@
package com.bakdata.conquery.io.jackson.serializer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

könntest du eine UtilityMethode machen, die Collection<TID extends Id<ID>> -> Collection<ID> abbildet?

...ava/com/bakdata/conquery/models/messages/namespaces/specific/UpdateElementMatchingStats.java
ConceptElementId<?> element = entry.getKey();
ConceptId conceptId = element.findConcept();

// mapping function cannot use Id::resolve here yet, somehow the nsIdResolver is not set because it
Copy link
Collaborator

Choose a reason for hiding this comment

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

das klingt nach einem Bug in dem Deserliazier? Vlt brauchst du einen dedizierten Key Deserializer

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 hatte ich noch nicht ganz verstanden. Ich war davon ausgegangen, dass der Normale Deserialiser genutzt wird, wenn kein KeyDeserializer genutzt wird. Aber es ist wohl am besten mit einem Test herauszufinden.

backend/src/test/java/com/bakdata/conquery/util/extensions/GroupExtension.java
import org.junit.jupiter.api.extension.ExtensionContext;

@Getter
public class GroupExtension implements BeforeAllCallback {
Copy link
Collaborator

Choose a reason for hiding this comment

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

was ist das?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ich habe das für einige Komponenten erstellt, die in einen Storage müssen, noch dem er geöffnet wurde, damit die Logik nicht in den @BeforeAll-Callback eines Tests muss und man es wiederverwenden kann.

Wir könnten diesen Extensions-Mechanismus auch um die Cached TestConqueries bauen um dann nicht mehr die DynamicNodes für ProgrammaticTests zu nutzen, sondern sie als echte Tests nutzen.
Der RestartTest könnte vermutlich auch DropwizardAppExtension nutzen

awildturtok
awildturtok reviewed Oct 1, 2024
View reviewed changes
@thoniTUB thoniTUB requested a review from awildturtok October 2, 2024 12:00
@awildturtok
Copy link
Collaborator

allgemeine bitte: Bitte squashe deinen merge, das ist dann schöner in der history.

@thoniTUB
Copy link
Collaborator Author

thoniTUB commented Oct 2, 2024

@awildturtok wir können sqash auch gerne als default machen

awildturtok
awildturtok approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@awildturtok awildturtok left a comment

Choose a reason for hiding this comment

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

Eine letzte Sache: Kannst du bitte einmal alle call-sites von resolve überprüfen ob sie in hotcode sind? Es ist unwahrscheinlich, aber es wäre schlecht wenn wir da so viel performance verballern.

backend/src/main/java/com/bakdata/conquery/apiv1/query/concept/specific/CQConcept.java
@@ -90,38 +94,38 @@ public class CQConcept extends CQElement implements NamespacedIdentifiableHoldin

public static CQConcept forSelect(Select select) {
final CQConcept cqConcept = new CQConcept();
cqConcept.setElements(List.of(select.getHolder().findConcept()));
cqConcept.setElements(List.of(select.getHolder().findConcept().getId()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

man könnte drüber nachdenken, ob du die ganze methode auf Id umstellen kannst. Mach mal bitte einen KOmmentar für später ran

backend/src/main/java/com/bakdata/conquery/apiv1/query/concept/specific/CQConcept.java
}

return cqConcept;
}

public static CQConcept forConnector(Connector source) {
final CQConcept cqConcept = new CQConcept();
cqConcept.setElements(List.of(source.getConcept()));
cqConcept.setElements(List.of(source.getConcept().getId()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

ebenso

backend/src/main/java/com/bakdata/conquery/io/jackson/serializer/IdDeserializer.java Outdated
Comment on lines 71 to 75
// We need to assign resolvers for namespaced and meta ids because meta-objects might reference namespaced objects (e.g. ExecutionsId)
NamespacedStorageProvider namespacedStorageProvider = NamespacedStorageProvider.getResolver(ctxt);
MetaStorage metaStorage = MetaStorage.get(ctxt);
setResolver(id, metaStorage, namespacedStorageProvider);
Copy link
Collaborator

Choose a reason for hiding this comment

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

kannst du das assignment aus dem try-catch Block nehmen? das wäre glaube ich sauberer?

backend/src/main/java/com/bakdata/conquery/io/jackson/serializer/IdDeserializer.java
Comment on lines +92 to +98
if (subId instanceof NamespacedId) {
subId.setNamespacedStorageProvider(namespacedStorageProvider);
}
else if (subId instanceof MetaId) {
subId.setMetaStorage(metaStorage);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blöde Frage, aber kannst du das nicht dem Interface überlassen? Also MetaId und NamespacedId sind Id<MetaStorage/NamespacedStorage> und haben ein Feld storageProvider? dann kannst du dir die ambiguation komplett sparen

backend/src/main/java/com/bakdata/conquery/models/events/Bucket.java
public Object createScriptValue(int event, @NotNull Column column) {
return getStore(column).createScriptValue(event);
}

public Map<String, Object> calculateMap(int event) {
final Map<String, Object> out = new HashMap<>(stores.length);
Column[] columns = getTable().resolve().getColumns();
Copy link
Collaborator

Choose a reason for hiding this comment

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

resolve immernoch in hot-loop

Copy link
Collaborator

Choose a reason for hiding this comment

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

calculateMap wird pro Zeile aufgerufen

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ich habe es jetzt in ein Lambda ausgelagert, dass pro bucket und cblock einmal generiert wird und stores und columns referenziert

backend/src/main/java/com/bakdata/conquery/models/events/BucketManager.java Outdated
private int getBucket(String id) {
return entity2Bucket.getInt(id);
}

/**
* Remove all buckets comprising the import. Which will in-turn remove all CBLocks.
*/
public void removeImport(Import imp) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

=> ImportId

@thoniTUB thoniTUB force-pushed the feature/remove-central-registry branch from c80118f to cbbfa29 Compare October 8, 2024 10:55
@thoniTUB thoniTUB enabled auto-merge October 8, 2024 10:55
@thoniTUB thoniTUB merged commit d775e0e into develop Oct 8, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awildturtok awildturtok awildturtok approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thoniTUB @awildturtok