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

[JBPM-9413] AMQ Streams (Kafka) integration Notifier / producer #1774

Merged
merged 1 commit into from Oct 21, 2020
Merged

[JBPM-9413] AMQ Streams (Kafka) integration Notifier / producer #1774

merged 1 commit into from Oct 21, 2020

Conversation

fjtirado
Copy link
Contributor

@fjtirado fjtirado commented Oct 8, 2020 •
edited

Defining kafka event emitter

Unit test included in this PR (integratin test will be included by QE)

JIRA:

JBPM-9413

Depends on
#1495

@fjtirado fjtirado force-pushed the JBPM-9413 branch 2 times, most recently from bbc6426 to 2c65b70 Compare October 9, 2020 13:00
@fjtirado fjtirado marked this pull request as ready for review October 9, 2020 14:14
@fjtirado
Copy link
Contributor Author

fjtirado commented Oct 9, 2020

jenkins retest this

@fjtirado fjtirado force-pushed the JBPM-9413 branch 6 times, most recently from 08cf65a to dad11ca Compare October 9, 2020 15:24
elguardian
elguardian requested changes Oct 13, 2020
View reviewed changes
Copy link
Member

@elguardian elguardian left a comment

Choose a reason for hiding this comment

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

The important one is the tx.
The rest are minor issues.

@fjtirado fjtirado force-pushed the JBPM-9413 branch 2 times, most recently from 8c89aac to eed9cc3 Compare October 13, 2020 12:20
@fjtirado fjtirado force-pushed the JBPM-9413 branch 6 times, most recently from 1193c45 to 9238546 Compare October 13, 2020 12:56
elguardian
elguardian approved these changes Oct 13, 2020
View reviewed changes
Copy link
Member

@elguardian elguardian left a comment

Choose a reason for hiding this comment

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

LGTM

@fjtirado fjtirado force-pushed the JBPM-9413 branch 2 times, most recently from 0a96bb9 to ca4768d Compare October 13, 2020 13:09
gmunozfe
gmunozfe approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@gmunozfe gmunozfe left a comment

Choose a reason for hiding this comment

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

Looks good to me, splendid work, just a few remarks if you want to consider them

...rs/jbpm-event-emitters-kafka/src/main/java/org/jbpm/event/emitters/kafka/CloudEvent_1_0.java Outdated Show resolved Hide resolved
...jbpm-event-emitters-kafka/src/main/java/org/jbpm/event/emitters/kafka/KafkaEventEmitter.java Show resolved Hide resolved
...jbpm-event-emitters-kafka/src/main/java/org/jbpm/event/emitters/kafka/KafkaEventEmitter.java Outdated Show resolved Hide resolved
...jbpm-event-emitters-kafka/src/main/java/org/jbpm/event/emitters/kafka/KafkaEventEmitter.java Show resolved Hide resolved
...-event-emitters-kafka/src/test/java/org/jbpm/event/emitters/kafka/KafkaEventEmitterTest.java Show resolved Hide resolved
gmunozfe
gmunozfe approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@gmunozfe gmunozfe left a comment

Choose a reason for hiding this comment

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

Looks good to me, well done @fjtirado !

@sonarcloud
Copy link

sonarcloud bot commented Oct 15, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

79.8% 79.8% Coverage
0.0% 0.0% Duplication

@fjtirado
Copy link
Contributor Author

@kiegroup/gatekeepers please merge this

@mareknovotny
Copy link
Member

please run FDB as good custome as there are multiple things tested in droolsjbpm-integration repo and probably outside too.

Jenkins do fdb

@mareknovotny
Copy link
Member

please look at 10 failures in kie-server integration tests

@fjtirado
Copy link
Contributor Author

fjtirado commented Oct 20, 2020 •
edited

please look at 10 failures in kie-server integration tests

@mareknovotny these 10 failed test are unrelated with this JIRA.
One group is failing because of a problem while downloading from repo could not transfer artifact org.default:artifact:pom:1.0.0 from/to jboss-public-repository-group (https://repository.jboss.org/nexus/content/groups/public/): Connection reset
Another group is related with
The grup related with translated_document seems to deserve investigation, but they are not defenitely related wit that change

@mareknovotny
Copy link
Member

yesterday were some problems with jboss.org Nexus trying to get another run as the top level FDB ran over 10 hours and timed out

Jenkins do fdb

@fjtirado
Copy link
Contributor Author

fjtirado commented Oct 20, 2020 •
edited

yesterday were some problems with jboss.org Nexus trying to get another run as the top level FDB ran over 10 hours and timed out

Jenkins do fdb

Now it failed without test results. In my opinion, this JIRA does not requires FDB, that why I did not execute it in the first place. Lets give it a third try

@elguardian elguardian merged commit 492be75 into kiegroup:master Oct 21, 2020
mareknovotny
mareknovotny reviewed Oct 21, 2020
View reviewed changes
jbpm-event-emitters/jbpm-event-emitters-kafka/pom.xml
<parent>
<groupId>org.jbpm</groupId>
<artifactId>jbpm-event-emitters</artifactId>
<version>7.45.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

@fjtirado @elguardian you need to fix the version as on master is already 7.46.0-SNAPSHOT, this breaks the master

Copy link
Member

Choose a reason for hiding this comment

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

fixed in c5796f6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mareknovotny mareknovotny mareknovotny left review comments

@gmunozfe gmunozfe gmunozfe approved these changes

@elguardian elguardian elguardian approved these changes

@afalhambra afalhambra Awaiting requested review from afalhambra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@fjtirado @mareknovotny @gmunozfe @elguardian