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

feat: allow environment variables to configure embedded connect #4260

Merged
merged 2 commits into from
Jan 10, 2020

Conversation

agavra
Copy link
Contributor

@agavra agavra commented Jan 9, 2020

Description

With this patch, docker users will be able to configure their embedded connect cluster using env variables prefixed with KSQL_CONNECT_.

Testing done

Spun up ksqldb using the following docker compose:

  ksqldb-server:
    image: placeholder/confluentinc/ksql-rest-app:local.build
    hostname: ksqldb-server
    container_name: ksqldb-server
    depends_on:
      - broker
      - schema-registry
    ports:
      - "8088:8088"
    environment:
      KSQL_BOOTSTRAP_SERVERS: "broker:9092"
      KSQL_HOST_NAME: ksqldb-server
      KSQL_LISTENERS: "http://0.0.0.0:8088"
      KSQL_CACHE_MAX_BYTES_BUFFERING: 0
      KSQL_KSQL_SCHEMA_REGISTRY_URL: "http://schema-registry:8081"
      KSQL_KSQL_LOGGING_PROCESSING_STREAM_AUTO_CREATE: "true"
      KSQL_KSQL_LOGGING_PROCESSING_TOPIC_AUTO_CREATE: "true"
      KSQL_LOG4J_OPTS: "-Dlog4j.configuration=file:/etc/ksql/log4j-rolling2.properties -Dlog4j.debug"
      KSQL_CONNECT_GROUP_ID: "ksql-connect-cluster"
      KSQL_CONNECT_BOOTSTRAP_SERVERS: "broker:9092"
      KSQL_CONNECT_KEY_CONVERTER: "io.confluent.connect.avro.AvroConverter"
      KSQL_CONNECT_KEY_CONVERTER_SCHEMA_REGISTRY: "http://schema-registry:8081"
      KSQL_CONNECT_VALUE_CONVERTER: "io.confluent.connect.avro.AvroConverter"
      KSQL_CONNECT_VALUE_CONVERTER_SCHEMA_REGISTRY: "http://schema-registry:8081"
      KSQL_CONNECT_CONFIG_STORAGE_TOPIC: "ksql-connect-configs"
      KSQL_CONNECT_OFFSET_STORAGE_TOPIC: "ksql-connect-offsets"
      KSQL_CONNECT_STATUS_STORAGE_TOPIC: "ksql-connect-statuses"
      KSQL_CONNECT_CONFIG_STORAGE_REPLICATION_FACTOR: 1
      KSQL_CONNECT_OFFSET_STORAGE_REPLICATION_FACTOR: 1
      KSQL_CONNECT_STATUS_STORAGE_REPLICATION_FACTOR: 1
    volumes:
      - ./log4j-rolling.properties:/etc/ksql/log4j-rolling2.properties

Also tested without a required param and it failed with the correct error message.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@agavra agavra requested a review from a team as a code owner January 9, 2020 22:54
rmoff
rmoff previously requested changes Jan 10, 2020
Copy link
Member

@rmoff rmoff left a comment

Choose a reason for hiding this comment

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

This is great, thanks @agavra !

Two things:

  1. I'd suggest is that we should stick to the KSQL prefix of all the other vars to be consistent
    e.g. KSQL_CONNECT_GROUP_ID rather than CONNECT_GROUP_ID
  2. We should include the script for checking required values https://github.com/confluentinc/kafka-images/blob/master/kafka-connect-base/include/etc/confluent/docker/configure

@agavra
Copy link
Contributor Author

agavra commented Jan 10, 2020

I'd suggest is that we should stick to the KSQL prefix of all the other vars to be consistent
e.g. KSQL_CONNECT_GROUP_ID rather than CONNECT_GROUP_ID

@rmoff - I thought about this, but I thought it would make more sense to omit KSQL as they are being passed directly to connect (and for the sake of brevity) but if you feel strongly then I don't (thoughts @derekjn @MichaelDrogalis?)

@MichaelDrogalis
Copy link
Contributor

I liked the reasoning behind @agavra's suggestion, too. But again I don't feel super strongly.

@derekjn
Copy link
Contributor

derekjn commented Jan 10, 2020

@rmoff - I thought about this, but I thought it would make more sense to omit KSQL as they are being passed directly to connect (and for the sake of brevity) but if you feel strongly then I don't (thoughts @derekjn @MichaelDrogalis?)

I personally prefer @rmoff's suggestion because it feels more embedded to me. KSQL_CONNECT indicates to me that Connect is subordinate to KSQL, and IMO it draws a sharper distinction between configuring an external Connect cluster versus running in embedded mode. And while these parameters are being passed through to Connect, I see that as an implementation detail that users don't necessarily need to be aware of.

That being said, I am also fine with either option here.

@MichaelDrogalis
Copy link
Contributor

@derekjn That seems like a convincing way to look at it to me. +1

@agavra agavra dismissed rmoff’s stale review January 10, 2020 18:06

Accepted both comments, will ship after green build

@agavra agavra force-pushed the connect_embedded_docker branch from 18a6bfd to c8e9bb1 Compare January 10, 2020 18:07
@agavra agavra merged commit e032ea9 into confluentinc:master Jan 10, 2020
@agavra
Copy link
Contributor Author

agavra commented Jan 10, 2020

Merging without dev review as this is docker-only build change

@agavra agavra deleted the connect_embedded_docker branch January 10, 2020 19:03
vpapavas added a commit to vpapavas/ksql that referenced this pull request Jan 14, 2020
vpapavas added a commit to vpapavas/ksql that referenced this pull request Jan 28, 2020
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.

4 participants