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 2021/05/14 23:42:38 UTC
[GitHub] [pulsar] david-streamlio opened a new pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
david-streamlio opened a new pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593
Fixes #10109
### Motivation
We are unable to create BatchSource connectors using the Pulsar Admin CLI, as there is no way to provide the batch source configuration other than providing it inside the source config yaml file. This fix provides an alternative means of providing these configuration vaules.
### Modifications
Modified org.apache.pulsar.admin.cli.CmdSources and created associated unit tests
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change added tests and can be verified as follows:
- Added unit tests that submit BatchSource class from the command line using the `--batch-source-config` switch
### 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): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: no)
- The rest endpoints: (no)
- The admin cli options: (yes)
- Anything that affects deployment: (no)
### Documentation
- Does this pull request introduce a new feature? (yes)
- If yes, how is the feature documented? (docs)
- 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] david-streamlio commented on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-843289624
/pulsarbot run-failure-checks
--
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] david-streamlio commented on a change in pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on a change in pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#discussion_r635452067
##########
File path: pulsar-common/src/test/java/org/apache/pulsar/common/io/BatchSourceConfigParseTest.java
##########
@@ -0,0 +1,53 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.io;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+
+import org.testng.annotations.Test;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+public class BatchSourceConfigParseTest {
+
+ private ObjectMapper objectMapper = new ObjectMapper();
+
+ @Test
+ public final void ImmediateTriggererTest() throws JsonMappingException, JsonProcessingException {
+ String json = "{ \"discoveryTriggererClassName\" : \"org.apache.pulsar.io.batchdiscovery.ImmediateTriggerer\" }";
Review comment:
@cckellogg This is what the JSON strings would look like for an ImmediateTriggerer.
--
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] david-streamlio commented on a change in pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on a change in pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#discussion_r635453727
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSources.java
##########
@@ -315,6 +316,8 @@ protected void validateSourceConfigs(SourceConfig sourceConfig) {
protected String DEPRECATED_sourceConfigString;
@Parameter(names = "--source-config", description = "Source config key/values")
protected String sourceConfigString;
+ @Parameter(names = "--batch-source-config", description = "Batch source config key/values")
+ protected String batchSourceConfigString;
Review comment:
@cckellogg So you are suggesting that we would use a combination of the `--batch-source-config` flag to specify the cron expression and another flag called `batch-trigger-type` that would take two values, Immediate and Cron ?
--
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] david-streamlio commented on a change in pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on a change in pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#discussion_r635452221
##########
File path: pulsar-common/src/test/java/org/apache/pulsar/common/io/BatchSourceConfigParseTest.java
##########
@@ -0,0 +1,53 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.io;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+
+import org.testng.annotations.Test;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+public class BatchSourceConfigParseTest {
+
+ private ObjectMapper objectMapper = new ObjectMapper();
+
+ @Test
+ public final void ImmediateTriggererTest() throws JsonMappingException, JsonProcessingException {
+ String json = "{ \"discoveryTriggererClassName\" : \"org.apache.pulsar.io.batchdiscovery.ImmediateTriggerer\" }";
+ BatchSourceConfig config = objectMapper.readValue(json, BatchSourceConfig.class);
+ assertNotNull(config);
+ assertEquals(config.getDiscoveryTriggererClassName(), "org.apache.pulsar.io.batchdiscovery.ImmediateTriggerer");
+ }
+
+ @Test
+ public final void CronTriggererTest() throws JsonMappingException, JsonProcessingException {
+ String json = "{ \"discoveryTriggererClassName\" : \"org.apache.pulsar.io.batchdiscovery.CronTriggerer\","
Review comment:
@cckellogg This is what the JSON strings would look like for a CronTriggerer
--
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] david-streamlio removed a comment on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio removed a comment on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-846156058
/pulsarbot run-failure-checks
--
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] cckellogg commented on a change in pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#discussion_r637120933
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSources.java
##########
@@ -315,6 +316,8 @@ protected void validateSourceConfigs(SourceConfig sourceConfig) {
protected String DEPRECATED_sourceConfigString;
@Parameter(names = "--source-config", description = "Source config key/values")
protected String sourceConfigString;
+ @Parameter(names = "--batch-source-config", description = "Batch source config key/values")
+ protected String batchSourceConfigString;
Review comment:
sounds good
--
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] david-streamlio commented on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-841701015
/pulsarbot run-failure-checks
--
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] david-streamlio commented on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-846156058
/pulsarbot run-failure-checks
--
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] david-streamlio commented on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-846278194
/pulsarbot run-failure-checks
--
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] david-streamlio removed a comment on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio removed a comment on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-841701015
/pulsarbot run-failure-checks
--
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] david-streamlio commented on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-842750154
/pulsarbot run-failure-checks
--
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 merged pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
jerrypeng merged pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593
--
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] david-streamlio commented on a change in pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on a change in pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#discussion_r637028453
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSources.java
##########
@@ -315,6 +316,8 @@ protected void validateSourceConfigs(SourceConfig sourceConfig) {
protected String DEPRECATED_sourceConfigString;
@Parameter(names = "--source-config", description = "Source config key/values")
protected String sourceConfigString;
+ @Parameter(names = "--batch-source-config", description = "Batch source config key/values")
+ protected String batchSourceConfigString;
Review comment:
@cckellogg If there was ever a new trigger type added to the project or via a separate class by a user, then we would need to change this class to perform the mapping from the "simple" name to the actual class name. I think it is best to leave it as `--batch-source-config` to be consistent with the `--source-config` switch that allows the user to send in all of the configs inside a single JSON object.
I will add an example of a valid JSON object to 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] david-streamlio removed a comment on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio removed a comment on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-841708486
/pulsarbot run-failure-checks
--
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] david-streamlio commented on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-843353597
/pulsarbot run-failure-checks
--
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] cckellogg commented on a change in pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#discussion_r634619714
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSources.java
##########
@@ -315,6 +316,8 @@ protected void validateSourceConfigs(SourceConfig sourceConfig) {
protected String DEPRECATED_sourceConfigString;
@Parameter(names = "--source-config", description = "Source config key/values")
protected String sourceConfigString;
+ @Parameter(names = "--batch-source-config", description = "Batch source config key/values")
+ protected String batchSourceConfigString;
Review comment:
Curious to what an example key/values string looks like? Is it a json string?
Also, wondering if another flag call trigger-type should exist? At this point all the implementations are internal and in my opinion the user shouldn't need to know full class name just what type they want to use. The cli tool can take this information and add the correct classname.
##########
File path: pulsar-common/pom.xml
##########
@@ -166,6 +166,11 @@
<scope>test</scope>
</dependency>
+ <dependency>
Review comment:
Is this needed?
--
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 #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#discussion_r634644263
##########
File path: pulsar-common/pom.xml
##########
@@ -166,6 +166,11 @@
<scope>test</scope>
</dependency>
+ <dependency>
Review comment:
Ya you probably don't need to import Gson. Just use Jackson i.e. `ThreadObjectFactory.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] david-streamlio removed a comment on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio removed a comment on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-842750154
/pulsarbot run-failure-checks
--
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] david-streamlio commented on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-844413248
/pulsarbot run-failure-checks
--
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] david-streamlio commented on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-841708486
/pulsarbot run-failure-checks
--
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] cckellogg commented on a change in pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#discussion_r636913174
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSources.java
##########
@@ -315,6 +316,8 @@ protected void validateSourceConfigs(SourceConfig sourceConfig) {
protected String DEPRECATED_sourceConfigString;
@Parameter(names = "--source-config", description = "Source config key/values")
protected String sourceConfigString;
+ @Parameter(names = "--batch-source-config", description = "Batch source config key/values")
+ protected String batchSourceConfigString;
Review comment:
I think a `batch-trigger-type` (with cron and immediate as values) and then maybe a `batch-cron-expression`. We could do this later in another patch. I think we should add some documentation and examples of how to pass the `--batch-source-config` because it's not obvious to me and I would need to look at the source code.
--
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] codelipenghui commented on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-845943265
@nlu90 Please help review this PR.
--
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] david-streamlio removed a comment on pull request #10593: [Issue-10109] [admin client] Add --batch-source-config switch to the Pulsar Admin Source API
Posted by GitBox <gi...@apache.org>.
david-streamlio removed a comment on pull request #10593:
URL: https://github.com/apache/pulsar/pull/10593#issuecomment-843289624
/pulsarbot run-failure-checks
--
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