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.

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

Added ability to add annotations to Connector Configs #6983

Merged
merged 15 commits into from May 22, 2020
Merged

Added ability to add annotations to Connector Configs #6983

merged 15 commits into from May 22, 2020

Conversation

srkukarni
Copy link
Contributor

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

This pr adds the ability for pulsar io sources and sinks to define annotations in their configs and have them checked at submission time. This means that creation/update of connectors with improper config can be rejected outright at request time rather than at runtime which is the current case. This will lead to faster troubleshooting when developing and running connectors. We make this check gated by a worker config parameter.

Modifications

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@srkukarni srkukarni added this to the 2.6.0 milestone May 19, 2020
@srkukarni srkukarni self-assigned this May 19, 2020
jerrypeng
jerrypeng reviewed May 20, 2020
View reviewed changes
pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java Outdated
ConnectorDefinition defn = ConnectorUtils.getConnectorDefinition(classLoader);
if (defn.getSinkConfigClass() != null) {
Class configClass = Class.forName(defn.getSinkConfigClass(), true, classLoader);
ObjectMapper mapper = new ObjectMapper();
Copy link
Contributor

@jerrypeng jerrypeng May 20, 2020 •
edited

Choose a reason for hiding this comment

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

Please use ObjectMapperFactory.getThreadLocal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

jerrypeng
jerrypeng reviewed May 20, 2020
View reviewed changes
pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java Outdated
if (defn.getSinkConfigClass() != null) {
Class configClass = Class.forName(defn.getSinkConfigClass(), true, classLoader);
ObjectMapper mapper = new ObjectMapper();
Object configObject = mapper.readValue(new ObjectMapper().writeValueAsString(sinkConfig.getConfigs()), configClass);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can achieve the same thing by using the following method

mapper.convertValue(map, MyPojo.class);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@jerrypeng
Copy link
Contributor

jerrypeng commented May 20, 2020 •
edited

@srkukarni thinking more about it, I am wondering for sources and sinks whether the following interface is appropriate now that we formalized the concept of a connector config.

    void open(final Map<String, Object> config, SourceContext sourceContext) throws Exception;

Since a connector will have a config class attached to it should we still have "Map<String, Object> config" as an argument or should we somehow just pass in the config POJO?

Though not sure that is possible given we only allow developers to specify in the connector config class in "pulsar-io.yaml". There is not way to use Java generics to automatically specify the type of the config argument.

@srkukarni
Copy link
Contributor Author

@jerrypeng wrt your comment on the correct interface, yes in hindsight it does seem as if we should have had an explicit config class in the open. Almost all of the connectors in the repo seem to be following this.
Having said that I also believe that the current approach gives more flexibility wrt how connector writers evolve their config in the future. I'm not sure if it's really worthwhile for us to change the api now for the elegance that the explicit config class provides. Its something that connector writers can live with without too much difficultly.

@srkukarni srkukarni requested a review from jerrypeng May 20, 2020 22:25
@srkukarni srkukarni merged commit c8170b7 into apache:master May 22, 2020
@srkukarni srkukarni deleted the connector_annotations branch May 22, 2020 15:24
nicolo-paganin pushed a commit to oncodeit/pulsar that referenced this pull request May 22, 2020
@srkukarni @nicolopaganin
Added ability to add annotations to Connector Configs (apache#6983)
46a5f9d 
* Added sourceConfigClass and sinkConfigClass

* Add Validator annotation helpers to validate class parameters

* Fix build errors

* Take feedback into account

* Connected with validation

* Fix bugs

* Added tests

* Fix class name

* Address feedback

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
nicolo-paganin pushed a commit to oncodeit/pulsar that referenced this pull request May 22, 2020
@srkukarni @nicolopaganin
Added ability to add annotations to Connector Configs (apache#6983)
344f875 
* Added sourceConfigClass and sinkConfigClass

* Add Validator annotation helpers to validate class parameters

* Fix build errors

* Take feedback into account

* Connected with validation

* Fix bugs

* Added tests

* Fix class name

* Address feedback

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
Huanli-Meng pushed a commit to Huanli-Meng/pulsar that referenced this pull request Jun 1, 2020
@srkukarni @Huanli-Meng
Added ability to add annotations to Connector Configs (apache#6983)
e52706e 
* Added sourceConfigClass and sinkConfigClass

* Add Validator annotation helpers to validate class parameters

* Fix build errors

* Take feedback into account

* Connected with validation

* Fix bugs

* Added tests

* Fix class name

* Address feedback

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
Huanli-Meng pushed a commit to Huanli-Meng/pulsar that referenced this pull request Jun 1, 2020
@srkukarni @Huanli-Meng
Added ability to add annotations to Connector Configs (apache#6983)
f9742c7 
* Added sourceConfigClass and sinkConfigClass

* Add Validator annotation helpers to validate class parameters

* Fix build errors

* Take feedback into account

* Connected with validation

* Fix bugs

* Added tests

* Fix class name

* Address feedback

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
Huanli-Meng pushed a commit to Huanli-Meng/pulsar that referenced this pull request Jun 12, 2020
@srkukarni @Huanli-Meng
Added ability to add annotations to Connector Configs (apache#6983)
92e4774 
* Added sourceConfigClass and sinkConfigClass

* Add Validator annotation helpers to validate class parameters

* Fix build errors

* Take feedback into account

* Connected with validation

* Fix bugs

* Added tests

* Fix class name

* Address feedback

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
merlimat pushed a commit to merlimat/pulsar that referenced this pull request Jun 30, 2020
@srkukarni
Added ability to add annotations to Connector Configs (apache#6983)
932e72d 
* Added sourceConfigClass and sinkConfigClass

* Add Validator annotation helpers to validate class parameters

* Fix build errors

* Take feedback into account

* Connected with validation

* Fix bugs

* Added tests

* Fix class name

* Address feedback

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@srkukarni @huangdx0726
Added ability to add annotations to Connector Configs (apache#6983)
da1b62e 
* Added sourceConfigClass and sinkConfigClass

* Add Validator annotation helpers to validate class parameters

* Fix build errors

* Take feedback into account

* Connected with validation

* Fix bugs

* Added tests

* Fix class name

* Address feedback

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerrypeng jerrypeng jerrypeng approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@murong00 murong00 Awaiting requested review from murong00

Assignees

@srkukarni srkukarni

Labels
area/connector
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@srkukarni @jerrypeng