search

confluentinc / cp-demo

Confluent Platform Demo including Apache Kafka, ksqlDB, Control Center, Schema Registry, Security, Schema Linking, and Cluster Linking
Apache License 2.0
38 stars 322 forks source link

[ci-skip] Set CLASSPATH Monitoring Interceptor jar to wildcard to catch any version (5.4.x only) #386

Closed imcdo closed 3 years ago

imcdo commented 3 years ago

Description

The class MonitoringConsumerInterceptor is not in the classpath by default, and is only available on the schema registry container. This update adds the internceptor to the classpath so that it can be used by the consumer. What behavior does this PR change, and why?

Author Validation

ran cp-demo start consumer script, validated that the consumergroup starts up

Reviewer Tasks

Describe the tasks/validation that the PR submitter is requesting to be done by the reviewer.

ybyzek commented 3 years ago

@imcdo please note https://confluentinc.atlassian.net/browse/DGS-72 and in light of that jira, it should mean that if CLASSPATH is set on the connect docker image (e.g. https://github.com/confluentinc/cp-demo/blob/5.4.x/docker-compose.yml#L358) , this PR should not be required. So first step would be to validate if the CLASSPATH is set properly for the connect image in this branch.

imcdo commented 3 years ago

@imcdo please note https://confluentinc.atlassian.net/browse/DGS-72 and in light of that jira, it should mean that if CLASSPATH is set on the connect docker image (e.g. https://github.com/confluentinc/cp-demo/blob/5.4.x/docker-compose.yml#L358) , this PR should not be required. So first step would be to validate if the CLASSPATH is set properly for the connect image in this branch.

Hey @ybyzek This is running in 5.4.5 patch release, and the classpath isn't set. Also the class itself only exists in the schema_registry image to begin with so there would still require some change to switch over to running on the schema registry container rather than connect. Unitl that ticket is addressed (as it seems like it hasn't been touched in a half a year or so) this is a way we can add it to the class path for this version of cp-demo for testing in the future.

ybyzek commented 3 years ago

@imcdo

  1. how have you validated that the MonitoringConsumerInterceptor class doesn't exist on the connect image? This doesn't sound right, how else does it run interceptors for its worker/connectors?
  2. My point in mentioning DGS-72 wasn't that it's going to get fixed, it's to highlight the conversation for you to ensure that the CLASSPATH is set correctly for the commandline clients to work.
imcdo commented 3 years ago

just checked all the jars to make sure, and no jar seems to have it in it.

ybyzek commented 3 years ago

@imcdo as discussed in Slack:

  1. You verified that jar does exist on the Connect image
  2. The workaround is not this PR. The workaround is to modify the CLASSPATH locally in your docker-compose file to whatever is shown in the output of docker-compose exec connect ls /usr/share/java/monitoring-interceptors/ or just change it to a wildcard like in 6.2.0-post branch.

Note: this issue is only on old branches. Latest branches like 6.2.0-post do NOT have this issue.

ybyzek commented 3 years ago

@imcdo please confirm final run-thru with latest commit and then I can merge (note to self: pint merge with the --skip-commits option so it doesn’t propagate (5.5.x already uses the wildcard: https://github.com/confluentinc/cp-demo/blob/5.5.x/docker-compose.yml#L391 so this does not need to be propagated to newer branches.)

imcdo commented 3 years ago

@imcdo please confirm final run-thru with latest commit and then I can merge (note to self: pint merge with the --skip-commits option so it doesn’t propagate (5.5.x already uses the wildcard: https://github.com/confluentinc/cp-demo/blob/5.5.x/docker-compose.yml#L391 so this does not need to be propagated to newer branches.)

@ybyzek looks good! validated cp-demo! let me know if you want me to do the merge