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

drop SqlManagedQuery, and instead move SQL concerns into SqlExecution… #3347

Merged
merged 9 commits into from
Mar 21, 2024

Conversation

awildturtok
Copy link
Collaborator

…Manager.

@awildturtok awildturtok force-pushed the feature/invert-control-for-ExecutionManager branch from b30af59 to 7c15a45 Compare March 19, 2024 12:52
@awildturtok
Copy link
Collaborator Author

Das ist ein ziemlich signifikanter Eingriff. Hoffnung ist, dass wir durch das invertieren alles an Logik zusammen ziehen können und nur noch die submission in die entsprechenden Query-Engines getrennt handlen müssen (ich weiß 🦄 ... aber.) Die vereinheitlichung fehlt noch, jetzt sollte @jnsrnhld aber in der Lage sein, Formulare einzureichen.

@@ -30,6 +30,6 @@ public class CollectQueryResult extends NamespaceMessage {
public void react(DistributedNamespace context) throws Exception {
log.info("Received {} of size {}", result, result.getResults().size());

context.getExecutionManager().handleQueryResult(result);
result.addResult(context.getExecutionManager() /*TODO*/);
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 nicht auch über eine ReactingMessage lösen?

@@ -177,17 +207,19 @@ public void clearQueryResults(ManagedExecution execution) {

@Override
public Stream<EntityResult> streamQueryResults(ManagedExecution execution) {
final List<List<EntityResult>> resultParts = executionResults.getIfPresent(execution.getId());
final Map<?, List<EntityResult>> resultParts = executionResults.getIfPresent(execution.getId());
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 ?, oder ist das egal?

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 ist die workerId, die ist an der Stelle egal.

c3a3055

in dem commit glaube nochmal klarer was passiert

Comment on lines -81 to -99
@Override
public void addResult(ShardResult result) {
log.debug("Received Result[size={}] for Query[{}]", result.getResults().size(), result.getQueryId());

log.trace("Received Result\n{}", result.getResults());

if (result.getError().isPresent()) {
fail(result.getError().get());
return;
}

involvedWorkers.remove(result.getWorkerId());

getNamespace().getExecutionManager().addQueryResult(this, result.getResults());

if (involvedWorkers.isEmpty() && getState() == ExecutionState.RUNNING) {
finish(ExecutionState.DONE);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Das ist gut, dass die Methode aus der Klasse fliegt

//TODO handle finishing etc here.
if (execution instanceof ManagedInternalForm<?> managedForm) {
managedForm.getSubQueries().values().forEach(subQuery -> doExecute(namespace, managedForm));
managedForm.finish(ExecutionState.DONE);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

du musst hier auch den state der subqueries checken :D wenn davon eine failed ist alles failed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Guter Punkt, hab es erstmal nur soweit gebracht dass bei mir ein FormTest im SQL-Mode durchläuft 😄

@awildturtok awildturtok merged commit 82e6829 into develop Mar 21, 2024
6 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feature/invert-control-for-ExecutionManager branch March 21, 2024 14:12
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.

None yet

3 participants