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/04 10:23:51 UTC
[GitHub] [pulsar] 315157973 opened a new pull request #6868: [issue #6765] Expose definition flags to function
315157973 opened a new pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868
Fixes #6765
### Motivation
### Modifications
1）
Change the value of the CLI tool parameter "--custom-schema-input" from
"TopicName-> schemaType"
to
"topicName-> {" schemaType ":" type"," jsr310ConversionEnabled ": true," alwaysAllowNull ": true}"
2）
Modify Function.proto, add properties "jsr310ConversionEnabled", "alwaysAllowNull"。So that we can receive the above 2 parameters
3）
Modify the "FunctionConfigUtils#convert" method ， put the two parameters in "CustomSchemaInputs" into ConsumerSpec
4）
In “JavaInstanceRunnable#setupInput” method, put the above 2 parameters into "ConsumerConfig" and pass it to "PulsarSource", let it set the parameters into "schema" when creating the consumer of Source。
So that，The “function” can get these 2 parameters from the message of
”currentRecord“
### 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? ( no)
- 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] sijie commented on a change in pull request #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#discussion_r436843900
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/SchemaDefinitionBuilderImpl.java
##########
@@ -35,7 +35,9 @@
public class SchemaDefinitionBuilderImpl<T> implements SchemaDefinitionBuilder<T> {
public static final String ALWAYS_ALLOW_NULL = "__alwaysAllowNull";
+ public static final String ALWAYS_ALLOW_NULL_NO_UNDERLINED = "alwaysAllowNull";
Review comment:
Why can't we use the existing settings?
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/SchemaDefinitionBuilderImpl.java
##########
@@ -35,7 +35,9 @@
public class SchemaDefinitionBuilderImpl<T> implements SchemaDefinitionBuilder<T> {
public static final String ALWAYS_ALLOW_NULL = "__alwaysAllowNull";
+ public static final String ALWAYS_ALLOW_NULL_NO_UNDERLINED = "alwaysAllowNull";
Review comment:
Can you explain why do you introduce two new settings without underscores?
----------------------------------------------------------------
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 #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#issuecomment-624678180
@nlu90 @gaoran10 If you have time, please also help take a look at this PR. @315157973 Thanks for your contribution.
----------------------------------------------------------------
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 #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#issuecomment-637376549
/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] 315157973 commented on a change in pull request #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#discussion_r436577000
##########
File path: pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/source/TopicSchema.java
##########
@@ -158,6 +173,20 @@ private static SchemaType getDefaultSchemaType(Class<?> clazz) {
}
}
+ private static <T> AvroSchema<T> buildAvroSchema(Class<T> clazz, ConsumerConfig conf) {
+ String jsr310ConversionEnabled = null;
+ String alwaysAllowNull = null;
+ if (conf.getSchemaProperties() != null) {
Review comment:
> Please use `SchemaDefinitionBuilder#withProperties` to construct the schema definition and avoid adding this kind of customization code. Also, the schema properties should be passed to other schemas as well.
Modify as follows:
1 Since the map generated by protobuf is immutable, a new Map is added to TopicSchema line 161.
2 Since the parameters such as JSR310 in SchemaDefinitionBuilder are underlined,the incoming parameters will be converted
@sijie
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/SchemaDefinitionBuilderImpl.java
##########
@@ -35,7 +35,9 @@
public class SchemaDefinitionBuilderImpl<T> implements SchemaDefinitionBuilder<T> {
public static final String ALWAYS_ALLOW_NULL = "__alwaysAllowNull";
+ public static final String ALWAYS_ALLOW_NULL_NO_UNDERLINED = "alwaysAllowNull";
Review comment:
Already use existing settings。This may cause some parameters to be underscores, some not
@sijie
----------------------------------------------------------------
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] 315157973 commented on a change in pull request #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#discussion_r429522194
##########
File path: pulsar-functions/proto/src/main/proto/Function.proto
##########
@@ -91,6 +91,8 @@ message ConsumerSpec {
int32 value = 1;
}
ReceiverQueueSize receiverQueueSize = 4;
+ bool jsr310ConversionEnabled = 5;
Review comment:
Properties have been merged into schemaProperties, and configuration is added, we can set the schema of SinkSpec
----------------------------------------------------------------
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] sijie commented on a change in pull request #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#discussion_r436418652
##########
File path: pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/source/TopicSchema.java
##########
@@ -158,6 +173,20 @@ private static SchemaType getDefaultSchemaType(Class<?> clazz) {
}
}
+ private static <T> AvroSchema<T> buildAvroSchema(Class<T> clazz, ConsumerConfig conf) {
+ String jsr310ConversionEnabled = null;
+ String alwaysAllowNull = null;
+ if (conf.getSchemaProperties() != null) {
Review comment:
Please use `SchemaDefinitionBuilder#withProperties` to construct the schema definition and avoid adding this kind of customization code. Also, the schema properties should be passed to other schemas as well.
----------------------------------------------------------------
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 #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#issuecomment-639201466
/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] 315157973 commented on a change in pull request #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#discussion_r436577000
##########
File path: pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/source/TopicSchema.java
##########
@@ -158,6 +173,20 @@ private static SchemaType getDefaultSchemaType(Class<?> clazz) {
}
}
+ private static <T> AvroSchema<T> buildAvroSchema(Class<T> clazz, ConsumerConfig conf) {
+ String jsr310ConversionEnabled = null;
+ String alwaysAllowNull = null;
+ if (conf.getSchemaProperties() != null) {
Review comment:
> Please use `SchemaDefinitionBuilder#withProperties` to construct the schema definition and avoid adding this kind of customization code. Also, the schema properties should be passed to other schemas as well.
Modify as follows:
1 Since the map generated by protobuf is immutable, a new Map is added to TopicSchema line 161.
2 Since the parameters such as JSR310 in SchemaDefinitionBuilder are underlined,the incoming parameters will be converted
----------------------------------------------------------------
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] 315157973 commented on pull request #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#issuecomment-636578629
Please review this pr again @sijie
----------------------------------------------------------------
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] sijie commented on a change in pull request #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#discussion_r426937125
##########
File path: pulsar-functions/proto/src/main/proto/Function.proto
##########
@@ -91,6 +91,8 @@ message ConsumerSpec {
int32 value = 1;
}
ReceiverQueueSize receiverQueueSize = 4;
+ bool jsr310ConversionEnabled = 5;
Review comment:
I don't think it is a good design to add these two settings directly. Because these two settings are only applied to AVRO or Struct schemas. It doesn't provide any extensibility to the framework of Pulsar functions. We should provide the ability to allow specifying `properties` for the schema for the input topic and `properties` for the schema for the output topic.
So ideally, it should be:
`map<string, string> schemaProperties = 6;`.
##########
File path: pulsar-functions/proto/src/main/proto/Function.proto
##########
@@ -91,6 +91,8 @@ message ConsumerSpec {
int32 value = 1;
}
ReceiverQueueSize receiverQueueSize = 4;
+ bool jsr310ConversionEnabled = 5;
Review comment:
We should also add the `schemaProperties` to both `SourceSpec` and `SinkSpec`.
----------------------------------------------------------------
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 merged pull request #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868
----------------------------------------------------------------
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 #6868: [issue #6765] Expose definition flags to function
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #6868:
URL: https://github.com/apache/pulsar/pull/6868#issuecomment-640539607
/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