Feature/resolve from storage #3441

thoniTUB · 2024-05-15T06:39:26Z

This pr makes the CentralRegistry obsolete, by resolving ids directly from the storage.

Before, the CentralResgistry held all objects in Memory so no memory optimized caching is possible.
Removing the registry removes one central point were hard references to all objects where held.

The CentralRegistry was used by deserializers (MetaIdReferenceDeserializer, NsIdReferenceDeserializer) to resolve incomming Ids in JSONs to concrete objects.
The CentralRegistry was however not a singleton, as every Storage (MetaStorage, NamespaceStorage, WorkerStorage) had one.
It is replaced by the NsIdResolver interface, which is implemented by NamespaceStorage, WorkerStorage and DatasetRegistry.

The MetaIdReferenceDeserializer just uses the MetaStorage directly.

Signed-off-by: Max Thonagel <[email protected]>

…istry

Signed-off-by: Max Thonagel <[email protected]>

thoniTUB · 2024-05-16T07:31:16Z

backend/src/main/java/com/bakdata/conquery/apiv1/query/concept/filter/FilterValue.java

Hier habe ich den Deserializer angepasst, so dass nicht erst zum Zeitpunkt der Deserializierung der IdResolver aus den Injectables rausgesucht wird, sondern schon während der Instantiierung

Signed-off-by: Max Thonagel <[email protected]>

thoniTUB · 2024-05-16T11:37:33Z

backend/src/main/java/com/bakdata/conquery/io/mina/BinaryJacksonCoder.java

@@ -18,7 +18,7 @@ public class BinaryJacksonCoder implements CQCoder<NetworkMessage<?>> {
 	private final ObjectWriter writer;
 	private final ObjectReader reader;

-	public BinaryJacksonCoder(IdResolveContext datasets, Validator validator, ObjectMapper objectMapper) {
+	public BinaryJacksonCoder(NsIdResolver datasets, Validator validator, ObjectMapper objectMapper) {


IdResolveContext konnte MetaId und NamespacedIds resolven. MetaIds werden aber gar nicht gebraucht in der Kommunikation zwischen Manager und Shards, deshalb gibt es hier nur einen NsIdResolver

thoniTUB · 2024-05-23T06:32:57Z

backend/src/main/java/com/bakdata/conquery/io/storage/MetaStorage.java

@@ -196,4 +197,24 @@ public void addFormConfig(FormConfig formConfig) {
 	public MutableInjectableValues inject(MutableInjectableValues values) {
 		return values.add(MetaStorage.class, this);
 	}
+
+	public <ID extends Id<VALUE>, VALUE extends Identifiable<?>> VALUE get(ID id) {


Diese get Methoden in den Storages imitieren einfach das ehemalöige getOptional der CentralRegistry.

Signed-off-by: Max Thonagel <[email protected]>

SebChmie

Sieht gut aus 😃

SebChmie · 2024-05-23T11:54:27Z

backend/src/main/java/com/bakdata/conquery/io/storage/NamespacedStorage.java

 		concepts.remove(id);
 	}

 	public Collection<Concept<?>> getAllConcepts() {
 		return concepts.getAll();
 	}

+	public <ID extends Id<VALUE> & NamespacedId, VALUE extends Identifiable<?>> VALUE get(ID id) {


Suggested change

public <ID extends Id<VALUE> & NamespacedId, VALUE extends Identifiable<?>> VALUE get(ID id) {

@SuppressWarnings("unchecked")

public <ID extends Id<VALUE> & NamespacedId, VALUE extends Identifiable<?>> VALUE get(ID id) {

SebChmie · 2024-05-23T11:57:22Z

backend/src/main/java/com/bakdata/conquery/io/storage/NamespacedStorage.java

-		super.clear();
-		centralRegistry.clear();
-	}
-
 	private void decorateDatasetStore(SingletonStore<Dataset> store) {


Wofür gibt es private void decorateDatasetStore, private void decorateSecondaryIdDescriptionStore, private void decorateTableStore und private void decorateImportStore

Die Methoden wurden für eine etwaige Initialisierung eines Objektes angelegt. Viele Objekte müssen aber nicht initialisiert werden und für andere habe ich denke ich eine bessere Lösung gefunden. Also die Funktionen können potentiell weg

SebChmie · 2024-05-23T12:24:50Z

backend/src/main/java/com/bakdata/conquery/models/datasets/PreviewConfig.java

@@ -114,19 +114,21 @@ public record InfoCardSelect(@NotNull String label, SelectId select, String desc
 	/**
 	 * Defines a group of selects that will be evaluated per quarter and year in the requested period of the entity-preview.
 	 */
-	public record TimeStratifiedSelects(@NotNull String label, String description, @NotEmpty List<InfoCardSelect> selects){
+	public record TimeStratifiedSelects(@NotNull String label, String description, @NotEmpty List<InfoCardSelect> selects) {
 	}

 	@ValidationMethod(message = "Selects may be referenced only once.")
 	@JsonIgnore
 	public boolean isSelectsUnique() {


Intellij meldet das diese ValidationMethods nicht benutzt werden.

oh das kann natürlich sein, mir wurde die Warnung nicht angezeigt.

Du musst Intellij einstellen, dass es bei dieser Anno kein unused meldet. Die werden automatisch ausgeführt bei Validierung

backend/src/main/java/com/bakdata/conquery/models/datasets/PreviewConfig.java

SebChmie · 2024-05-23T12:25:57Z

backend/src/main/java/com/bakdata/conquery/models/datasets/PreviewConfig.java

 	}

 	@ValidationMethod(message = "Labels must be unique.")
 	@JsonIgnore
 	public boolean isLabelsUnique() {
-		return timeStratifiedSelects.stream().map(TimeStratifiedSelects::selects).flatMap(Collection::stream).map(InfoCardSelect::label).distinct().count() == timeStratifiedSelects.stream().map(TimeStratifiedSelects::selects).flatMap(Collection::stream).count();
+		return timeStratifiedSelects.stream().map(TimeStratifiedSelects::selects).flatMap(Collection::stream).map(InfoCardSelect::label).distinct().count()
+			   == timeStratifiedSelects.stream().map(TimeStratifiedSelects::selects).flatMap(Collection::stream).count();


Suggested change

== timeStratifiedSelects.stream().map(TimeStratifiedSelects::selects).flatMap(Collection::stream).count();

== timeStratifiedSelects.stream().map(TimeStratifiedSelects::selects).mapToLong(Collection::size).sum();

…viewConfig.java Co-authored-by: SebChmie <[email protected]>

awildturtok

-300 Zeilen code und es ist jetzt direkter gekoppelt, nice!

awildturtok · 2024-06-04T08:21:15Z

backend/src/main/java/com/bakdata/conquery/apiv1/query/concept/filter/FilterValue.java

@@ -197,13 +202,21 @@ public void resolve(QueryResolveContext context) {
 	 * <p>
 	 * The resolved filter instructs the frontend on how to render and serialize the filter value using the {@link Filter#createFrontendConfig(com.bakdata.conquery.models.config.ConqueryConfig)} method. The filter must implement {@link GroupFilter} and provide the type information of the value to correctly deserialize the received object.
 	 */
-	public static class GroupFilterDeserializer extends StdDeserializer<GroupFilterValue> {
-		private final NsIdReferenceDeserializer<FilterId, Filter<?>> nsIdDeserializer = new NsIdReferenceDeserializer<>(Filter.class, null, FilterId.class);
+	public static class GroupFilterDeserializer extends StdDeserializer<GroupFilterValue> implements ContextualDeserializer {


Ich weiß, es ist bisschen dein Baby, aber kann das nicht auch ganz weg?

awildturtok · 2024-06-04T08:22:08Z

backend/src/main/java/com/bakdata/conquery/io/jackson/MetaIdRefParamConverter.java


 	@Override
 	public VALUE fromString(String value) {
 		final ID id = idParser.parse(value);

-		return registry.resolve(id);
+		return storage.get(id);


awildturtok · 2024-06-04T08:26:47Z

backend/src/main/java/com/bakdata/conquery/io/storage/IdentifiableCachedStore.java

@@ -1,77 +0,0 @@
-package com.bakdata.conquery.io.storage;


gelöscth, weil du caching verschieben willst?

awildturtok · 2024-06-04T08:28:27Z

backend/src/main/java/com/bakdata/conquery/io/storage/MetaStorage.java

@@ -196,4 +191,24 @@ public void addFormConfig(FormConfig formConfig) {
 	public MutableInjectableValues inject(MutableInjectableValues values) {
 		return values.add(MetaStorage.class, this);
 	}
+
+	public <ID extends Id<VALUE>, VALUE extends Identifiable<?>> VALUE get(ID id) {
+		if (id instanceof ManagedExecutionId executionId) {


können nicht die ids das getten machen?

awildturtok · 2024-06-04T08:31:06Z

backend/src/main/java/com/bakdata/conquery/io/storage/NamespaceStorage.java

+
+	@Override
+	public <ID extends Id<VALUE> & NamespacedId, VALUE extends Identifiable<?>> VALUE get(ID id) {
+		if (id instanceof InternToExternMapperId castId) {


würde den access den ids überlassen

awildturtok · 2024-06-04T09:07:12Z

backend/src/main/java/com/bakdata/conquery/models/worker/DatasetRegistry.java

+        datasetStorage.close();
+
+		return createNamespace(datasetStorage, metaStorage);


Formatierun stimmt hier nicht

awildturtok · 2024-06-04T09:08:23Z

backend/src/main/java/com/bakdata/conquery/models/worker/DatasetRegistry.java

+        return values.add(NsIdResolver.class, this)
+                .add(this.getClass(), this);


sind beide injections notwendig?

awildturtok · 2024-06-04T09:10:20Z

backend/src/test/java/com/bakdata/conquery/api/form/config/FormConfigTest.java

-		doAnswer(invocation -> {
-			throw new UnsupportedOperationException("Not yet implemented");
-		}).when(namespacesMock).getOptional(any());
+		// TODO


noch offen?

awildturtok · 2024-06-04T09:12:01Z

backend/src/test/java/com/bakdata/conquery/models/SerializationTests.java

+		final WorkerStorage workerStorage = getWorkerStorage();

-		CentralRegistry registry = getMetaStorage().getCentralRegistry();
-
-		registry.register(dataset);
-		registry.register(startCol);
-		registry.register(endCol);
-		registry.register(compoundCol);
-		registry.register(table);
-		registry.register(imp);
-		registry.register(bucket);
+		workerStorage.updateDataset(dataset);
+		workerStorage.addTable(table);
+		workerStorage.addImport(imp);
+		workerStorage.addBucket(bucket);


awildturtok · 2024-06-04T09:13:01Z

backend/src/test/java/com/bakdata/conquery/models/types/SerialisationObjectsUtil.java

 	@NotNull
-	public static TreeConcept createConcept(CentralRegistry registry, Dataset dataset) {
+	public static TreeConcept createConcept(Dataset dataset, NamespacedStorage ... storages) {


wieso können das mehrere Storages sein?

thoniTUB and others added 12 commits May 14, 2024 12:31

use metastorage directly for resolving ids

2df80a5

Signed-off-by: Max Thonagel <[email protected]>

wip: resolve ids from storage

2f406dd

Signed-off-by: Max Thonagel <[email protected]>

wip comment in test

6af5294

Signed-off-by: Max Thonagel <[email protected]>

wraps serdes exception to give information about used object mapper view

aaa14a0

injects namespacesstorage into test api mapper to override datasetreg…

873160f

…istry

calculate column position in table

ee5e394

fix SerializationTests

ec18d38

fix most filter tests

7edbcfb

Signed-off-by: Max Thonagel <[email protected]>

fixing json tests

028d802

Signed-off-by: Max Thonagel <[email protected]>

further fixing json tests

04bc939

Signed-off-by: Max Thonagel <[email protected]>

fixes programmatic tests

49d0c15

fixes more tests

9fa3e55

thoniTUB commented May 16, 2024

View reviewed changes

thoniTUB added 3 commits May 16, 2024 09:34

only inject into object mapper if provided

e6d15bb

Signed-off-by: Max Thonagel <[email protected]>

remove cache from serdes as it is to early

4725afa

Signed-off-by: Max Thonagel <[email protected]>

removes CentralRegistry and similar classes

2eea202

Signed-off-by: Max Thonagel <[email protected]>

thoniTUB commented May 16, 2024

View reviewed changes

thoniTUB commented May 23, 2024

View reviewed changes

thoniTUB added 7 commits May 23, 2024 08:37

adds override annotations to get methods

08813c7

Signed-off-by: Max Thonagel <[email protected]>

remove obsolete registration of idresolver in tests

f36a9ca

Signed-off-by: Max Thonagel <[email protected]>

remove comment

27ceb90

Signed-off-by: Max Thonagel <[email protected]>

adds comment

80c4200

Signed-off-by: Max Thonagel <[email protected]>

removes DirectIdentifiableStore.java

075684f

Signed-off-by: Max Thonagel <[email protected]>

update doc string

51385c3

Signed-off-by: Max Thonagel <[email protected]>

decouple DatasetRegistry and MetaStorage more

6a8d96e

Signed-off-by: Max Thonagel <[email protected]>

thoniTUB requested a review from SebChmie May 23, 2024 09:15

Merge branch 'develop' into feature/resolve-from-storage

086cec1

SebChmie approved these changes May 23, 2024

View reviewed changes

Update backend/src/main/java/com/bakdata/conquery/models/datasets/Pre…

98f80a1

…viewConfig.java Co-authored-by: SebChmie <[email protected]>

awildturtok requested changes Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/resolve from storage #3441

Feature/resolve from storage #3441

thoniTUB commented May 15, 2024 •

edited

Loading

thoniTUB May 16, 2024

thoniTUB May 16, 2024

thoniTUB May 23, 2024

SebChmie left a comment

SebChmie May 23, 2024

SebChmie May 23, 2024

thoniTUB May 29, 2024

SebChmie May 23, 2024

thoniTUB May 29, 2024

awildturtok Jun 4, 2024

SebChmie May 23, 2024

awildturtok left a comment

awildturtok Jun 4, 2024

awildturtok Jun 4, 2024

awildturtok Jun 4, 2024

awildturtok Jun 4, 2024

awildturtok Jun 4, 2024

awildturtok Jun 4, 2024

awildturtok Jun 4, 2024

awildturtok Jun 4, 2024

awildturtok Jun 4, 2024

awildturtok Jun 4, 2024

	== timeStratifiedSelects.stream().map(TimeStratifiedSelects::selects).flatMap(Collection::stream).count();
	== timeStratifiedSelects.stream().map(TimeStratifiedSelects::selects).mapToLong(Collection::size).sum();

		datasetStorage.close();

		return createNamespace(datasetStorage, metaStorage);

		return values.add(NsIdResolver.class, this)
		.add(this.getClass(), this);

Feature/resolve from storage #3441

Are you sure you want to change the base?

Feature/resolve from storage #3441

Conversation

thoniTUB commented May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebChmie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awildturtok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thoniTUB commented May 15, 2024 •

edited

Loading