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

Java client accepts topologies with DEAD partitions #9410

Merged
merged 2 commits into from
May 23, 2022
Merged

Java client accepts topologies with DEAD partitions #9410

merged 2 commits into from
May 23, 2022

Conversation

lenaschoenburg
Copy link
Member

Description

The Java client now accepts partitions that are DEAD instead of throwing an exception. I've also added a test that generates randomized topology responses and ensures that they are accepted by the Java client.

closes #9387

@lenaschoenburg lenaschoenburg force-pushed the 9387-java-client-dead-partitions branch from 3085888 to 1c15228 Compare May 18, 2022 16:34
@lenaschoenburg
Copy link
Member Author

@saig0 Since you reacted to the original issue, are you interested in reviewing this? 😉

@saig0 saig0 self-requested a review May 19, 2022 03:12
@lenaschoenburg
Copy link
Member Author

Hm, looks like I can't get Junit 5 to work for the java-client module 🤔

[2022-05-19T06:14:48.670Z] [ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M6:test (default-test) on project zeebe-client-java: 
[2022-05-19T06:14:48.670Z] [ERROR] 
[2022-05-19T06:14:48.670Z] [ERROR] Please refer to /home/jenkins/agent/workspace/9387-java-client-dead-partitions/clients/java/target/surefire-reports for the individual test results.
[2022-05-19T06:14:48.670Z] [ERROR] Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
[2022-05-19T06:14:48.670Z] [ERROR] There was an error in the forked process
[2022-05-19T06:14:48.670Z] [ERROR] TestEngine with ID 'junit-jupiter' failed to discover tests
[2022-05-19T06:14:48.670Z] [ERROR] org.apache.maven.surefire.booter.SurefireBooterForkException: There was an error in the forked process
[2022-05-19T06:14:48.670Z] [ERROR] TestEngine with ID 'junit-jupiter' failed to discover tests
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.plugin.surefire.booterclient.ForkStarter.fork(ForkStarter.java:699)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.plugin.surefire.booterclient.ForkStarter.run(ForkStarter.java:311)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.plugin.surefire.booterclient.ForkStarter.run(ForkStarter.java:268)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeProvider(AbstractSurefireMojo.java:1332)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeAfterPreconditionsChecked(AbstractSurefireMojo.java:1165)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.plugin.surefire.AbstractSurefireMojo.execute(AbstractSurefireMojo.java:929)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:137)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:210)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:156)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:148)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:117)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.lifecycle.internal.builder.multithreaded.MultiThreadedBuilder$1.call(MultiThreadedBuilder.java:196)
[2022-05-19T06:14:48.670Z] [ERROR] 	at org.apache.maven.lifecycle.internal.builder.multithreaded.MultiThreadedBuilder$1.call(MultiThreadedBuilder.java:186)
[2022-05-19T06:14:48.671Z] [ERROR] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[2022-05-19T06:14:48.671Z] [ERROR] 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
[2022-05-19T06:14:48.671Z] [ERROR] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[2022-05-19T06:14:48.671Z] [ERROR] 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[2022-05-19T06:14:48.671Z] [ERROR] 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[2022-05-19T06:14:48.671Z] [ERROR] 	at java.lang.Thread.run(Thread.java:750)

@lenaschoenburg
Copy link
Member Author

Well, that's too bad. I can't really use easy-random for the tests because the Java client needs to be Java 8 compatible and easy-random 5.0 requires Java 11. I also can't use two different versions since that is a dependency convergence issue.

@saig0 What do you think, should I just remove the test and leave only the fix?

saig0
saig0 requested changes May 20, 2022
View reviewed changes
Copy link
Member

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@oleschoenburg thank you for providing a fix 👍

I have suggestion about the test. Please have a look.

clients/java/src/test/java/io/camunda/zeebe/client/impl/response/TopologyImplTest.java Outdated Show resolved Hide resolved
clients/java/pom.xml Outdated Show resolved Hide resolved
@lenaschoenburg lenaschoenburg force-pushed the 9387-java-client-dead-partitions branch from 832b68e to eaf3cda Compare May 20, 2022 14:27
@lenaschoenburg lenaschoenburg requested a review from saig0 May 20, 2022 14:28
saig0
saig0 approved these changes May 23, 2022
View reviewed changes
Copy link
Member

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@oleschoenburg LGTM 👍

@saig0
Copy link
Member

saig0 commented May 23, 2022

bors merge

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit e0ba2b3 into main May 23, 2022
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the 9387-java-client-dead-partitions branch May 23, 2022 03:54
@backport-action backport-action mentioned this pull request May 23, 2022
Merged
@backport-action
Copy link
Collaborator

Must have admin rights to Repository.

@backport-action backport-action mentioned this pull request May 23, 2022
Merged
@backport-action
Copy link
Collaborator

Must have admin rights to Repository.

@saig0
Copy link
Member

saig0 commented May 23, 2022

@korthout something is wrong with the backport action 😅 can you have a look?

@korthout
Copy link
Member

Thanks for letting me know. It seems to be related to the request review feature. To request reviews, the @backport-action user must have admin rights to this repo, which it doesn't have. I'll open a bug for it.

As a workaround in zeebe, until a fix is available, we could give the backport-action user admin rights to the repo. For example, by adding it to the Zeebe team. However, I don't think we should because the account is not controlled by Camunda, and therefore less secured. Luckily it seems to open PRs correctly.

I think the only solution is to disable the feature or make enabling it configurable. However, that will require a new release.

@korthout
Copy link
Member

A new version is released https://github.com/zeebe-io/backport-action/releases/tag/v0.0.8

@backport-action
Copy link
Collaborator

Git push to origin failed for stable/1.3 with exitcode 1

@backport-action
Copy link
Collaborator

Git push to origin failed for stable/8.0 with exitcode 1

@korthout
Copy link
Member

Haha wow, it picked up the url as a backport comment. You can ignore these last 2 comments.

zeebe-bors-camunda bot added a commit that referenced this pull request May 24, 2022
@zeebe-bors-camunda @lenaschoenburg
merge: #9435
54d3ada 
9435: [Backport stable/1.3] Java client accepts topologies with `DEAD` partitions r=saig0 a=backport-action

# Description
Backport of #9410 to `stable/1.3`.

relates to #9387

Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
zeebe-bors-camunda bot added a commit that referenced this pull request May 24, 2022
@zeebe-bors-camunda @lenaschoenburg
merge: #9436
9c28a36 
9436: [Backport stable/8.0] Java client accepts topologies with `DEAD` partitions r=saig0 a=backport-action

# Description
Backport of #9410 to `stable/8.0`.

relates to #9387

Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saig0 saig0 saig0 approved these changes

Assignees
No one assigned
Labels
version:1.3.9
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Java client fails to describe partitions that are DEAD
5 participants
@lenaschoenburg @saig0 @backport-action @korthout @npepinpe