You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/05/19 06:55:53 UTC
[GitHub] [pulsar] srkukarni opened a new pull request #6983: Added ability to add annotations to Connector Configs
srkukarni opened a new pull request #6983:
URL: https://github.com/apache/pulsar/pull/6983
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
*(If this PR fixes a github issue, please add `Fixes #<xyz>`.)*
Fixes #<xyz>
*(or if this PR is one task of a github issue, please add `Master Issue: #<xyz>` to link to the master issue.)*
Master Issue: #<xyz>
### 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng edited a comment on pull request #6983: Added ability to add annotations to Connector Configs
Posted by GitBox <gi...@apache.org>.
jerrypeng edited a comment on pull request #6983:
URL: https://github.com/apache/pulsar/pull/6983#issuecomment-631748364
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on pull request #6983: Added ability to add annotations to Connector Configs
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on pull request #6983:
URL: https://github.com/apache/pulsar/pull/6983#issuecomment-631748364
@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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #6983: Added ability to add annotations to Connector Configs
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #6983:
URL: https://github.com/apache/pulsar/pull/6983#discussion_r428328609
##########
File path: pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java
##########
@@ -580,4 +595,24 @@ public static SinkConfig validateUpdate(SinkConfig existingConfig, SinkConfig ne
}
return mergedConfig;
}
+
+ public static void validateConnectorConfig(SinkConfig sinkConfig, ClassLoader classLoader) {
+ try {
+ ConnectorDefinition defn = ConnectorUtils.getConnectorDefinition(classLoader);
+ 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);
Review comment:
You can achieve the same thing by using the following method
```
mapper.convertValue(map, MyPojo.class);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] srkukarni commented on a change in pull request #6983: Added ability to add annotations to Connector Configs
Posted by GitBox <gi...@apache.org>.
srkukarni commented on a change in pull request #6983:
URL: https://github.com/apache/pulsar/pull/6983#discussion_r428338513
##########
File path: pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java
##########
@@ -580,4 +595,24 @@ public static SinkConfig validateUpdate(SinkConfig existingConfig, SinkConfig ne
}
return mergedConfig;
}
+
+ public static void validateConnectorConfig(SinkConfig sinkConfig, ClassLoader classLoader) {
+ try {
+ ConnectorDefinition defn = ConnectorUtils.getConnectorDefinition(classLoader);
+ if (defn.getSinkConfigClass() != null) {
+ Class configClass = Class.forName(defn.getSinkConfigClass(), true, classLoader);
+ ObjectMapper mapper = new ObjectMapper();
Review comment:
Changed
##########
File path: pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java
##########
@@ -580,4 +595,24 @@ public static SinkConfig validateUpdate(SinkConfig existingConfig, SinkConfig ne
}
return mergedConfig;
}
+
+ public static void validateConnectorConfig(SinkConfig sinkConfig, ClassLoader classLoader) {
+ try {
+ ConnectorDefinition defn = ConnectorUtils.getConnectorDefinition(classLoader);
+ 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);
Review comment:
Changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] srkukarni commented on pull request #6983: Added ability to add annotations to Connector Configs
Posted by GitBox <gi...@apache.org>.
srkukarni commented on pull request #6983:
URL: https://github.com/apache/pulsar/pull/6983#issuecomment-631759029
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #6983: Added ability to add annotations to Connector Configs
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #6983:
URL: https://github.com/apache/pulsar/pull/6983#discussion_r428327699
##########
File path: pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java
##########
@@ -580,4 +595,24 @@ public static SinkConfig validateUpdate(SinkConfig existingConfig, SinkConfig ne
}
return mergedConfig;
}
+
+ public static void validateConnectorConfig(SinkConfig sinkConfig, ClassLoader classLoader) {
+ try {
+ ConnectorDefinition defn = ConnectorUtils.getConnectorDefinition(classLoader);
+ if (defn.getSinkConfigClass() != null) {
+ Class configClass = Class.forName(defn.getSinkConfigClass(), true, classLoader);
+ ObjectMapper mapper = new ObjectMapper();
Review comment:
Please use ObjectMapperFactor.getThreadLocal
##########
File path: pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java
##########
@@ -580,4 +595,24 @@ public static SinkConfig validateUpdate(SinkConfig existingConfig, SinkConfig ne
}
return mergedConfig;
}
+
+ public static void validateConnectorConfig(SinkConfig sinkConfig, ClassLoader classLoader) {
+ try {
+ ConnectorDefinition defn = ConnectorUtils.getConnectorDefinition(classLoader);
+ if (defn.getSinkConfigClass() != null) {
+ Class configClass = Class.forName(defn.getSinkConfigClass(), true, classLoader);
+ ObjectMapper mapper = new ObjectMapper();
Review comment:
Please use ObjectMapperFactory.getThreadLocal
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] srkukarni merged pull request #6983: Added ability to add annotations to Connector Configs
Posted by GitBox <gi...@apache.org>.
srkukarni merged pull request #6983:
URL: https://github.com/apache/pulsar/pull/6983
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org