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

Zeebe java client didn't get the region from the configuration #11846

Closed
ingorichtsmeier opened this issue Feb 28, 2023 · 7 comments · Fixed by #11870
Closed

Zeebe java client didn't get the region from the configuration #11846

ingorichtsmeier opened this issue Feb 28, 2023 · 7 comments · Fixed by #11870
Assignees
Labels
good first issue Marks an issue as simple enough for first time contributors kind/bug Categorizes an issue or PR as a bug onboarding version:8.1.9 Marks an issue as being completely or in parts released in 8.1.9 version:8.2.0 Marks an issue as being completely or in parts released in 8.2.0

Comments

@ingorichtsmeier
Copy link
Contributor

Describe the bug

If I set up a java client for Zeebe with the CloudClientBuilder, it will only connect to the bru-2 cluster. I cannot use this client for other regions.

To Reproduce

Build a client with

    zeebeClientBuilder = ZeebeClient.newCloudClientBuilder()
          .withClusterId(config.getProperty(ClientProperties.CLOUD_CLUSTER_ID))
          .withClientId(config.getProperty(ClientProperties.CLOUD_CLIENT_ID))
          .withClientSecret(ClientProperties.CLOUD_CLIENT_SECRET);      
    zeebeClient = zeebeClientBuilder.withProperties(config).build();
    LOG.info("Creating ZeebeClient using {}", zeebeClientBuilder);
        
    Topology topology = zeebeClient.newTopologyRequest().send().join();
    LOG.info("Cluster Topology: {}", topology);

and properties

zeebe.client.cloud.clusterId=xxx
zeebe.client.cloud.clientId=xxx
zeebe.client.cloud.secret=xxx
zeebe.client.cloud.region=bru-3

Expected behavior
Running this should cause an exception, as bru-3 doesn't exist (at least for Zeebe).

Instead, it returns the topology from my cluster in bru-2.

Environment:

  • OS: Windows
  • Zeebe Version: 8.1.0
  • Configuration: see above
@ingorichtsmeier ingorichtsmeier added the kind/bug Categorizes an issue or PR as a bug label Feb 28, 2023
@ingorichtsmeier
Copy link
Contributor Author

After inspecting the tests, I saw that bru-2 is handled as the default region: https://github.com/camunda/zeebe/blob/main/clients/java/src/test/java/io/camunda/zeebe/client/ZeebeClientTest.java#L248-L265

Is this still valid as we support more regions and the default region for Free clusters is North America?

There is a property for the region missing in io.camunda.zeebe.client.ClientProperties.

Would this be the fix to add the property?

@korthout korthout added good first issue Marks an issue as simple enough for first time contributors onboarding labels Mar 1, 2023
@korthout
Copy link
Member

korthout commented Mar 1, 2023

This is a great issue to get started with contributing to Zeebe. It's quite clear what needs to happen: add region property to the client configuration properties.

It may teach you about how the java client works, and how it can be configured to connect to a cluster.

There are workarounds available: use the .withRegion method to set the region, or use the spring-zeebe-client.

@ogkunald
Copy link
Contributor

ogkunald commented Mar 1, 2023

Hello @korthout please review the PR ^_^.Thanks!

@ingorichtsmeier
Copy link
Contributor Author

Hi @ogkunald,

I think that you should add some more tests on this topic here: https://github.com/camunda/zeebe/blob/main/clients/java/src/test/java/io/camunda/zeebe/client/ZeebeClientTest.java

What happens, if a value for your new property is given? -> The client should connect to the given region
What happens, if no value for your new property is given? -> The client should connect to bru-2

My expectation is, that especially this statement zeebeClientBuilder.withProperties(config).build(); will work for another region than bru-2.

@korthout
Copy link
Member

korthout commented Mar 1, 2023

Thanks @ingorichtsmeier You beat me to it, but that would be what I would ask for as well.

@ogkunald Please have a look at our Contributing guide for details on how to build from source and run the tests

@ogkunald
Copy link
Contributor

ogkunald commented Mar 2, 2023

#11870
As per the suggestion above in thread, I have added couple of tests-

  1. value for cloud region property is provided , client will connect to region provided
  2. value for cloud region property is not provided ,client will connect to default (bru-2) region.

Please review and revert in-case of anything.

@ingorichtsmeier
Copy link
Contributor Author

Hi @ogkunald, looks like as it was my own suggestion, thank you ;-)

I could not approve it, but @korthout can do it now.

And I can remove that topic from my list.

@ghost ghost closed this as completed in a13af2e Mar 3, 2023
ghost pushed a commit that referenced this issue Mar 3, 2023
11837: [Backport stable/8.0] fix: don't re-wrap runtime exceptions thrown inside a transaction r=oleschoenburg a=oleschoenburg

Manual backport of #11701 without the last commit which contained a test that is not straight forward to re-implement without the stream-platform abstraction.

11915: [Backport stable/8.0] added region property to client properties r=korthout a=backport-action

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

relates to #11846

Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
Co-authored-by: ogkunald <kunaldongare111@gmail.com>
ghost pushed a commit that referenced this issue Mar 3, 2023
11916: [Backport stable/8.1] added region property to client properties r=korthout a=backport-action

# Description
Backport of #11870 to `stable/8.1`.

relates to #11846

Co-authored-by: ogkunald <kunaldongare111@gmail.com>
ghost pushed a commit that referenced this issue Mar 3, 2023
11837: [Backport stable/8.0] fix: don't re-wrap runtime exceptions thrown inside a transaction r=oleschoenburg a=oleschoenburg

Manual backport of #11701 without the last commit which contained a test that is not straight forward to re-implement without the stream-platform abstraction.

11915: [Backport stable/8.0] added region property to client properties r=korthout a=backport-action

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

relates to #11846

Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
Co-authored-by: ogkunald <kunaldongare111@gmail.com>
Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
@megglos megglos added the version:8.1.9 Marks an issue as being completely or in parts released in 8.1.9 label Mar 13, 2023
@npepinpe npepinpe added the version:8.2.0 Marks an issue as being completely or in parts released in 8.2.0 label Apr 5, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Marks an issue as simple enough for first time contributors kind/bug Categorizes an issue or PR as a bug onboarding version:8.1.9 Marks an issue as being completely or in parts released in 8.1.9 version:8.2.0 Marks an issue as being completely or in parts released in 8.2.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants