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

Feature/decision polling #142

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

Conversation

tDalile
Copy link
Collaborator

@tDalile tDalile commented Jul 12, 2022

Added decision polling. Related to: #62 (comment)

@tDalile tDalile marked this pull request as ready for review July 12, 2022 10:16
private String decisionRequirementsDefinitionId;
private String decisionRequirementsDefinitionKey;
private Double collectResultValue;

Copy link

Choose a reason for hiding this comment

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

Leerzeile zu viel, oder ist das extra? Die anderen Typen im Projekt haben keine freien Zeilen zwischen den Attributen, es allerdings auch noch keine Listen oder Maps. 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rein sinngemäß habe ich hier eine Leerzeile gemacht, kann aber bei Bedarf wieder weg.

private String removalTime;
private String rootProcessInstanceId;
private String value;

Copy link

Choose a reason for hiding this comment

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

s.o.

Comment on lines -9 to +12
*
*
*
*
* @author viadee
*
*
Copy link

Choose a reason for hiding this comment

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

Wenn das das Einzige ist, was in dieser Datei geändert wird, dann m.E. reverten, damit sich nicht "so viel" ändert?

Comment on lines -11 to +14
*
*
*
*
* @author viadee
*
*
Copy link

Choose a reason for hiding this comment

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

s.o., ff.

Comment on lines 240 to 251
<dependency>
<groupId>de.viadee.camunda</groupId>
<artifactId>camunda-kafka-model</artifactId>
<version>2.1.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>de.viadee.camunda</groupId>
<artifactId>camunda-kafka-model</artifactId>
<version>2.1.0</version>
<scope>compile</scope>
</dependency>
Copy link

Choose a reason for hiding this comment

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

Das Model ist eigentlich bereits als Abhängigkeit in der Pom, ein paar Zeilen weiter oben, und warum ist es 2x hier drin? ;)

Comment on lines 37 to 38
logging.level.de.viadee.camunda.kafka.pollingclient=INFO No newline at end of file
logging.level.de.viadee.camunda.kafka.pollingclient=DEBUG
Copy link

Choose a reason for hiding this comment

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

Default sollte m.E. Info bleiben!

import de.viadee.camunda.kafka.event.IdentityLinkEvent;
import de.viadee.camunda.kafka.event.HistoryEvent;
import de.viadee.camunda.kafka.event.ProcessInstanceEvent;
import de.viadee.camunda.kafka.event.*;
Copy link

Choose a reason for hiding this comment

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

s.o.

@@ -605,6 +616,57 @@ static Stream<Arguments> pollIdentityLinks() {
// @formatter:on
}

@DisplayName("Polling of decision Instances")
Copy link

Choose a reason for hiding this comment

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

Instances s.o.

.stream()
.filter(event -> event instanceof DecisionInstanceEvent)
.map(event -> ((DecisionInstanceEvent) event).getOutputs())
.findFirst();
Copy link

Choose a reason for hiding this comment

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

Du könntest das findFirstersetzten z.B. mit .or(() -> Assertions.fail("Decision instance expected, but nothing found"));, dann sparst du dir das Optional.get().

Comment on lines +231 to +241
<dependency>
<groupId>org.camunda.bpm.dmn</groupId>
<artifactId>camunda-engine-dmn</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.camunda.bpm.dmn</groupId>
<artifactId>camunda-engine-dmn-bom</artifactId>
<scope>test</scope>
</dependency>
Copy link

Choose a reason for hiding this comment

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

In das nicht bereits mit der camunda-bom abgedeckt?


// query xml
try {
String xml = IOUtils.toString(repositoryService.getResourceAsStream(decisionDefinition.getDeploymentId(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Können wir hier Probleme mit dem Encoding bekommen?

@mschulte
Copy link

mschulte commented Aug 9, 2022

@MTwelkemeier, @rnschk : Könntet ihr nochmal ein Review machen? Ein baldiger Merge würde @tDalile in der Weiterentwicklung sehr helfen.

@fkoehne
Copy link
Contributor

fkoehne commented Aug 9, 2022

Disclaimer: Michael ist in Elternzeit. Wir werden Geduld oder Mut brauchen.

@tDalile
Copy link
Collaborator Author

tDalile commented Aug 9, 2022

Dann doch lieber Geduld. Einen funktionierenden Workaround für die Weiterentwicklung habe ich bereits.

@tDalile tDalile force-pushed the feature/decision-polling branch from 6975570 to 44e882d Compare August 25, 2022 11:58
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.

5 participants