Skip to content

Conversation

@awalther28
Copy link
Contributor

Looking into the single test failure but wanted folks input while investigating. Looks like the same test is consistently failing in master as well 🤔 (link to master semaphore tests)

This PR addresses #562. It proposes the following:

  • remove schema registry's zookeeper dependency in docker-compose
  • swap SCHEMA_REGISTRY_KAFKASTORE_CONNECTION_URL: 'zookeeper:2181' for SCHEMA_REGISTRY_KAFKASTORE_BOOTSTRAP_SERVERS: 'PLAINTEXT://broker:9092'
  • alter test for merging/ksql DESCRIBE EXTENDED diff check
    • investigation with @bbejeck into the matter, appears changes to configs cause expected behavior (local running stats weren't printed out previously and now they are)

@gAmUssA
Copy link

gAmUssA commented Oct 12, 2020

💯 for removing zookeeper config in SR!

Copy link
Contributor

@ybyzek ybyzek left a comment

Choose a reason for hiding this comment

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

🏅 @awalther28 great improvement across the board. Left 2 comments: one missed ZK connect param that can be deleted, and one for consideration.

@ybyzek
Copy link
Contributor

ybyzek commented Oct 12, 2020

There is one more tutorial that refers to something ZooKeeper-related:

_includes/tutorials/connect-add-key-to-source/kstreams/code/docker-compose.yml:      CONNECT_LOG4J_LOGGERS: org.apache.zookeeper=ERROR,org.I0Itec.zkclient=ERROR,org.reflections=ERROR      

Background: I wrote that tutorial and borrowed that configuration line from other GitHub examples. Admittedly, this was a copy/paste, and so I don't know its purpose or what the impact would be to remove it.

Do we know if that needs to be there? Would org.reflections=ERROR be sufficient?

Or is this a trivial/moot point because ZK can still be removed from Docker Compose (with KIP-500) and this would continue to work?

@awalther28
Copy link
Contributor Author

@ybyzek to your question about log4j filters... given the lack of reply from the connect team and use of those zk filters in other contexts, do we want to keep them for the time being?

@ybyzek
Copy link
Contributor

ybyzek commented Oct 13, 2020

@awalther28 agree, let's leave them as-is.

@bbejeck
Copy link
Contributor

bbejeck commented Oct 13, 2020

As discussed offline with @awalther28 the failure is unrelated to this PR.

@awalther28 awalther28 merged commit 6d7bbb8 into master Oct 13, 2020
@awalther28 awalther28 deleted the GH-562 branch October 13, 2020 22:57
colinhicks pushed a commit that referenced this pull request May 13, 2021
GH #562: Remove config dependency on ZooKeeper (not ZooKeeper the service)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove config dependency on ZooKeeper (not ZooKeeper the service)

5 participants