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: add a DefaultSchemaRegistryClient and remove default for SR url in KsqlConfig #4325

Merged
merged 1 commit into from
Jan 16, 2020

Conversation

stevenpyzhang
Copy link
Member

@stevenpyzhang stevenpyzhang commented Jan 16, 2020

Description

Fixes #4319

Removed default URL for SR from KsqlConfig
Implemented a new DefaultSchemaRegistryClient that's used when the URL is ""

ksql> CREATE STREAM pageviews_avro WITH (KAFKA_TOPIC='pageviews', VALUE_FORMAT='AVRO');
Schema registry fetch for topic pageviews request failed.
Caused by: KSQL is not configured to use a schema registry. To enable it, please
        set ksql.schema.registry.url

Testing done

Unit test
Verified logs weren't being spammed when closing queries

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 #")

@stevenpyzhang stevenpyzhang requested a review from a team as a code owner January 16, 2020 00:03
Copy link
Contributor

@rodesai rodesai left a comment

Choose a reason for hiding this comment

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

LGTM. One comment inline.

@stevenpyzhang stevenpyzhang force-pushed the sr-logging branch 2 times, most recently from 5e33a1a to 847ee27 Compare January 16, 2020 19:01
@stevenpyzhang
Copy link
Member Author

Reworked the error message to be pluggable with the ErrorMessages interface.

@stevenpyzhang stevenpyzhang force-pushed the sr-logging branch 2 times, most recently from ad2cc75 to fa47b9f Compare January 16, 2020 19:47
@stevenpyzhang
Copy link
Member Author

stevenpyzhang commented Jan 16, 2020

While reworking this to have the pluggable error message, it seems like it might be better to move the config from KsqlRestConfig to KsqlConfig, thoughts @rodesai @spena ? Also, tagging @big-andy-coates since he was reviewer of original PR adding the config.

ErrorMessages seems generic enough to apply to any KSQL user case, not restricted to the rest application.

Copy link
Contributor

@rodesai rodesai left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenpyzhang stevenpyzhang merged commit e045f7c into confluentinc:master Jan 16, 2020
@vcrfxia
Copy link
Contributor

vcrfxia commented Jan 17, 2020

Sorry I'm late to the party, but this is a breaking change, right? If I understand correctly, previously users didn't need to explicitly set a config for KSQL to pick up a Schema Registry cluster at localhost:8081 but now they do. We should:

  • edit the git commit message or otherwise reflect this so it's picked up by the changelog
  • loop in someone from devX to make sure we haven't broken demos/examples

@rodesai
Copy link
Contributor

rodesai commented Jan 17, 2020

This change broke confluent-security-plugins. I've submitted a PR with a workaround (https://github.com/confluentinc/confluent-security-plugins/pull/257), but it's not a real fix to the problem because if a user uses RBAC they can no longer set a customized error message. I think we should either:

  • move ErrorMessages to KsqlConfig and construct it correctly in confluent-security-plugins, or:
  • refactor this change to have DefaultSchemaRegistryClient throw a custom exception without the special error message, and have the rest endpoint look out for this exception to set the final error message (like we do for auth errors).

My preference would be the latter since it keeps the usage of the Errors interface more isolated.

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.

Avoid spamming logs with errors about SR if SR not configured
4 participants