Skip to content

Remove ZooKeeperClient/KafkaZkClient usage from tests #893

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
Merged
merged 1 commit into from
Sep 7, 2021
Merged

Remove ZooKeeperClient/KafkaZkClient usage from tests #893

merged 1 commit into from
Sep 7, 2021

Conversation

ijuma
Copy link
Member

@ijuma ijuma commented Sep 7, 2021 •
edited
Loading

These are internal classes and hence can change in incompatible ways.
One such incompatible change committed to Apache Kafka 3.0 is the reason
why we need this in 7.0.x too.

@ijuma
Remove ZooKeeperClient/KafkaZkClient usage from tests
d2be8dd 
These are internal classes and hence can change in incompatible ways.
One such incompatible change committed to Apache 3.0 is the reason
why we need this in 7.0.x too.
@ijuma
Copy link
Member Author

ijuma commented Sep 7, 2021

One test failed for build 2:

io.confluent.kafkarest.v2.KafkaConsumerManagerTest.testBackoffMsControlsPollCalls
https://jenkins.confluent.io/job/Confluentinc%20Contributors/job/kafka-rest/job/PR-893/2/

This looks to be flaky, it fails occasionally without this change. And it passed locally after a few retries.

rondagostino
rondagostino approved these changes Sep 7, 2021
View reviewed changes
Copy link
Contributor

@rondagostino rondagostino left a comment

Choose a reason for hiding this comment

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

LGTM assuming builds pass.

@ijuma ijuma merged commit f0595b9 into 7.0.x Sep 7, 2021
@ijuma ijuma deleted the remove-zk-client-usage branch September 7, 2021 21:28
ijuma added a commit that referenced this pull request Sep 7, 2021
@ijuma
Merge remote-tracking branch 'origin/7.0.x'
b439b1d 
* origin/7.0.x:
  Remove ZooKeeperClient/KafkaZkClient usage from tests (#893)
@dimitarndimitrov
Copy link
Member

This seems to have caused all integration tests based on ClusterTestHarness to fail for 7.0.x and master. As a unit test, KafkaConsumerManagerTest failing seems to have hidden all of these by skipping their phase. I'll provide an update when I have a fix available, hopefully soon.

dimitarndimitrov added a commit to dimitarndimitrov/kafka-rest that referenced this pull request Sep 8, 2021
@dimitarndimitrov
Fix NPE causing most integration tests to fail
b263ab2 
Recent AK changes related to the removal of ZooKeeper have
necessitated the accompanying changes in kafka-rest's tests.
confluentinc#893 has done
that, but unfortunately it also removed the initialization of
the zkConnect field which then caused an NPE when it ended
up being added to a ConcurrentHashMap as a null.

This attempts to fix the problem by the simplest of approaches:
putting back the initialization of zkConnect.
@dimitarndimitrov dimitarndimitrov mentioned this pull request Sep 8, 2021
Merged
dimitarndimitrov added a commit that referenced this pull request Sep 8, 2021
@dimitarndimitrov
Fix NPE causing most integration tests to fail (#895)
69ece06 
Recent AK changes related to the removal of ZooKeeper have
necessitated the accompanying changes in kafka-rest's tests.
#893 has done
that, but unfortunately it also removed the initialization of
the zkConnect field which then caused an NPE when it ended
up being added to a ConcurrentHashMap as a null.

This attempts to fix the problem by the simplest of approaches:
putting back the initialization of zkConnect.
dimitarndimitrov added a commit to dimitarndimitrov/kafka-rest that referenced this pull request Sep 8, 2021
@dimitarndimitrov
KREST-1965 Ignore some known very flaky tests
095c521 
The two tests ignored here are way too flaky and we have seen at least
one occasion were one of them was hiding an even worse problem:
confluentinc#893.

We have added tasks for actually fixing them, but in the meantime
they should not be causing as much trouble as they currently are.
@dimitarndimitrov dimitarndimitrov mentioned this pull request Sep 8, 2021
Merged
@ijuma
Copy link
Member Author

ijuma commented Sep 8, 2021

Sorry for that. Can we fix the build not to ignore integration tests in such cases?

@dimitarndimitrov
Copy link
Member

Sorry for that. Can we fix the build not to ignore integration tests in such cases?

I don't think you (or anyone) could have done anything better in this specific situation.

As for not ignoring other phases, we should very much look into that.
I am currently experimenting with the following: (1) comment all but one unit tests and all but one integration tests; (2) reproduce the skipping locally by failing the unit test; (3) test whether running with --fail-at-end also runs the integration test;

dimitarndimitrov added a commit that referenced this pull request Sep 8, 2021
@dimitarndimitrov
KREST-1965 Ignore some known very flaky tests (#896)
ad9bdae 
The two tests ignored here are way too flaky and we have seen at least
one occasion were one of them was hiding an even worse problem:
#893.

We have added tasks for actually fixing them, but in the meantime
they should not be causing as much trouble as they currently are.
@rigelbm rigelbm mentioned this pull request Sep 9, 2021
Merged
rigelbm added a commit that referenced this pull request Sep 10, 2021
@rigelbm
KREST-2260: Use super user when creating topic in AuthorizationErrorT…
1085071 
…est. (#897)

Prior to #893, this topic was being created directly in Zookeeper, bypassing Kafka authorization entirely.

Now that we are using Kafka to create the topic, we need to pass through authorization. This changes uses the super-user to create the topic, so that we don't fail authorization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rondagostino rondagostino rondagostino approved these changes

@rigelbm rigelbm Awaiting requested review from rigelbm

@agg111 agg111 Awaiting requested review from agg111

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

Successfully merging this pull request may close these issues.
3 participants
@ijuma @dimitarndimitrov @rondagostino