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.

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

use caffeine in CachedStore and adapt ManagedInternalForm for state p… #3607

Open
wants to merge 53 commits into
base: develop
Choose a base branch
from

Conversation

thoniTUB
Copy link
Collaborator

…ersistence

@thoniTUB thoniTUB requested a review from awildturtok October 21, 2024 17:51
@@ -258,6 +254,9 @@ public synchronized void finish(ExecutionState executionState) {

getExecutionManager().updateState(getId(), executionState);

// Persist state of this execution
metaStorage.updateExecution(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

wird finish nicht immer vom execution Manager aufgerufen, so dass der das schon machen sollte?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ja schon, aber der Zustand, wann eine Execution fertig ist unterscheidet sich zwischen SQL und Distributed, so dass man es hier an einer zentralen stelle abhaken kann. Auch sind wir hier in einen synchronized Block. Den müsste man dann weiter rauszeihen.

@thoniTUB thoniTUB marked this pull request as ready for review November 8, 2024 07:37
@thoniTUB
Copy link
Collaborator Author

thoniTUB commented Nov 8, 2024

@awildturtok Kannst du bitte nochmal drüber schauen, das Laden der Stores ist jetzt konfigurierbar, wo bei der Bucket und CBlock dennoch bei jedem Start direkt geladen werden durch den BucketManager

Und ich habe den LoadStorageTask abgeändert, so dass er im ManagerNode, ShardNode und Standalone genutzt werden kann.

# Conflicts:
#	backend/src/main/java/com/bakdata/conquery/commands/ManagerNode.java
#	backend/src/main/java/com/bakdata/conquery/models/forms/managed/ExternalExecution.java
@thoniTUB thoniTUB requested a review from awildturtok January 8, 2025 08:06
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.

Kopf raucht. Bin noch nicht durch.

* @implNote If you implement this method, please do it always from scratch and not using calls to super, it can be quite annoying.
*/
public abstract ImmutableList<ManagedStore> getStores();
public interface ConqueryStorage extends Closeable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wo hilft es, dass das jetzt ein interface ist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dadurch konnte ich einfach den XodusStore einfach ein einen CachedStore wrappen

@@ -385,8 +393,8 @@ public IterationStatistic forEach(StoreEntryConsumer<KEY, VALUE> consumer) {
maybeFailed = allJobs.get(30, TimeUnit.SECONDS);
}
catch (InterruptedException e) {
Thread.interrupted();
log.debug("Thread was interrupted.");
boolean interrupted = Thread.interrupted();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sollten wir bei einem Interrupted nicht aufräumen und alles stoppen?

Momentan machen wir ja einfach weiter. Ich denke es wäre am einfachsten die InterruptedException in eine RuntimeException wrappen

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 teste mit isInterrupted und rethrowe wenn das wirklich so ist

@@ -53,7 +55,8 @@ public ByteIterable get(ByteIterable key) {
*
* @param consumer function called for-each key-value pair.
*/
public void forEach(BiConsumer<ByteIterable, ByteIterable> consumer) {
@Override
public SerializingStore.IterationStatistic forEach(StoreEntryConsumer<ByteIterable, ByteIterable> consumer) {
AtomicReference<ByteIterable> lastKey = new AtomicReference<>();
AtomicBoolean done = new AtomicBoolean(false);
while (!done.get()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

solltest du dann hier nicht auch den iterator verwenden?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Die sind sehr auf den SerializingStore gemünzt. Dass die hier der Return sind ist leaky

@thoniTUB thoniTUB requested a review from awildturtok January 22, 2025 07:43
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.

das binary caching stört mich immer noch aber da wir es mit hoher Wahrscheinlichkeit nicht benutzen werden, ist es mir der diskussion nicht wert

@@ -71,7 +76,10 @@ public void execute(String name, TestConquery testConquery) throws Exception {

final Query query = IntegrationUtils.parseQuery(conquery, test.getRawQuery());

final long nImports = namespace.getStorage().getAllImports().count();
final long nImports;
try(Stream<Import> allImports = namespace.getStorage().getAllImports()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

recherchier mal bitte ob du die streams überhaupt schließen musst, terminating operations sollten den auch schon schließen

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 dazu diesen Kommentar gefunden: https://stackoverflow.com/a/28814024

Es ist wohl unüblich Streams aus releasebaren Resourcen auszugeben. Hier sollten wir vielleicht nochmal die StoreApi ändern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants