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

Use admin client to confirm cluster is correctly started #24

Merged
merged 12 commits into from
Feb 16, 2023
Merged

Use admin client to confirm cluster is correctly started #24

merged 12 commits into from
Feb 16, 2023

Conversation

SamBarker
Copy link
Member

Uses an admin client to describe the cluster and ensure the expected number of brokers are included before injecting into tests.

The one snag this faces is when authentication comes into play.

Initially I tried to connect to the inter-broker listener however that's not currently exposed to the host when running in test containers. So switched back to using the client listener. When that is authenticated the default admin client is unable to connect.

I see three possible solutions.

  1. Always generate an additional "admin" account which the extension can use to connect
  2. Ensure the inter broker listener is exposed outside of the VM network
  3. Create an additional listener without authentication requirements for the admin client to connect too.

Separately I think there is some additional refactoring to do around how endpoints are defined and used to generate the various different formats of host and port required. i.e. clients don't like resolving //localhost:9093 and listeners don't like localhost:9093.

Fixes #7

@SamBarker
Unified bootstrap string generation
37694ae 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Ensure correct number of brokers are started before injecting clusters
c948586 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Fix style issues
2766f83 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@k-wall
Copy link
Contributor

k-wall commented Feb 15, 2023

Option 3 seems least worst to me. I prefer the clean separation of concerns that a dedicated listener gives us. It will be easier to identify in the logs too, which I expect we'll appreciate when debugging thorny issues.

@SamBarker
Use try with resources to ensure the client is closed.
f7268b6 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Make buildBootstrapServers private
006f39b 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Introduce listenAddress and advertisedAddress to the endpoint.
af8b4cf 
Signed-off-by: Sam Barker <sbarker@redhat.com>
tombentley
tombentley approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

I left a few nits, but otherwise LGTM.

impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterConfig.java Outdated Show resolved Hide resolved
impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterConfig.java Outdated Show resolved Hide resolved
impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterConfig.java Outdated Show resolved Hide resolved
impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterConfig.java
@@ -388,11 +434,23 @@ public interface KafkaEndpoints {
class EndpointPair {
private final Endpoint bind;
private final Endpoint connect;

public String connectAddress() {
return String.format("%s:%d", connect.host, connect.port);
Copy link
Contributor

Choose a reason for hiding this comment

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

The %d is locale-dependent, according to the "Number Localization Algorithm" section in Formatter Javadoc, which says:

Each digit character d in the string is replaced by a locale-specific digit computed relative to the current locale's zero digit z;

To avoid this I would use concatenation using +.

Copy link
Contributor

Choose a reason for hiding this comment

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

public char getZeroDigit()
Gets the character used for zero. Different for Arabic, etc.

Wow, I never knew that. I've written so much code that must fall on its face in the Arabic world.

impl/src/main/java/io/kroxylicious/testing/kafka/invm/InVMKafkaCluster.java
}

@Override
public EndpointPair getControllerEndpoint(int brokerId) {
// TODO why can't we treat ZK as the controller port outside of kraft mode?
Copy link
Contributor

Choose a reason for hiding this comment

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

Open an issue if this is worthy of further investigation.

Copy link
Member Author

@SamBarker SamBarker Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Opened #25 replied to wrong comment.

impl/src/main/java/io/kroxylicious/testing/kafka/invm/InVMKafkaCluster.java Outdated Show resolved Hide resolved
impl/src/main/java/io/kroxylicious/testing/kafka/common/Utils.java Outdated Show resolved Hide resolved
k-wall
k-wall approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@k-wall k-wall 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.

SamBarker and others added 4 commits February 17, 2023 10:28
@SamBarker
Add an anonymous listener for the extension to ensure it can always d…
e476870 
…ial into the cluster

Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @tombentley
typo fix
713d51b 
Co-authored-by: Tom Bentley <tombentley@users.noreply.github.com>
@SamBarker
ensureExpectedBrokerCountInCluster -> `awaitExpectedBrokerCountInCl…
208737e 
…uster`

Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
RedHat -> kroxylicious.io
4f7529e 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker SamBarker marked this pull request as ready for review February 16, 2023 21:36
@SamBarker
b -> controllerId
06823d6 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
update impsort config and reformat.
8475323 
Matches with kroxylicious-parent.

Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker SamBarker merged commit ee5966b into kroxylicious:main Feb 16, 2023
@SamBarker SamBarker deleted the issue-7-using_awaitility branch February 16, 2023 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tombentley tombentley tombentley approved these changes

@k-wall k-wall k-wall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ParameterExtensionTest is flaky / race condition
3 participants
@SamBarker @k-wall @tombentley