-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Set key for message when using function publish #4005
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
Changes from all commits
2e84172
8885b0c
b5792b6
90aa999
e59e730
7f0d7c5
46ce383
993c382
95d1cfd
ba9b884
dea473a
35a4640
45f79de
03be813
34361ae
bcae09c
d58330e
332405d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,15 +118,43 @@ def get_secret(self, secret_name): | |
| """Returns the secret value associated with the name. None if nothing was found""" | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def get_partition_key(self): | ||
| """Returns partition key of the input message is one exists""" | ||
| pass | ||
|
|
||
|
|
||
| @abstractmethod | ||
| def record_metric(self, metric_name, metric_value): | ||
| """Records the metric_value. metric_value has to satisfy isinstance(metric_value, numbers.Number)""" | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def publish(self, topic_name, message, serde_class_name="serde.IdentitySerDe", properties=None, compression_type=None, callback=None): | ||
| """ | ||
|
|
||
| DEPRECATED | ||
|
|
||
| Publishes message to topic_name by first serializing the message using serde_class_name serde | ||
| The message will have properties specified if any | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure we want to always do it this way. For example, what if we have multiple input topics with different keys, etc.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that should be the default behavior if the user doesn't explicitly specify the key since that is the behavior for objects returned from the function. We can add an additional method in which the user can explicitly specify a key |
||
| """ | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def publish(self, topic_name, message, serde_class_name="serde.IdentitySerDe", compression_type=None, callback=None, message_conf=None): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: any reason why do we have two methods here? doesn't the named argument
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sijie are you talking about and The former is for backwards compatibility purposes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in python you don't need two methods for backwards compatibility though. one method with named argument is good enough |
||
| """Publishes message to topic_name by first serializing the message using serde_class_name serde | ||
| The message will have properties specified if any""" | ||
| The message will have properties specified if any | ||
|
|
||
| The available options for message_conf: | ||
|
|
||
| properties, | ||
| partition_key, | ||
| sequence_id, | ||
| replication_clusters, | ||
| disable_replication, | ||
| event_timestamp | ||
|
|
||
| """ | ||
| pass | ||
|
|
||
| @abstractmethod | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,7 @@ public interface Context { | |
|
|
||
| /** | ||
| * The id of the function that we are executing | ||
| * | ||
| * @return The function id | ||
| */ | ||
| String getFunctionId(); | ||
|
|
@@ -119,16 +120,17 @@ public interface Context { | |
| /** | ||
| * Increment the builtin distributed counter referred by key. | ||
| * | ||
| * @param key The name of the key | ||
| * @param key The name of the key | ||
| * @param amount The amount to be incremented | ||
| */ | ||
| void incrCounter(String key, long amount); | ||
|
|
||
|
|
||
| /** | ||
| * Increment the builtin distributed counter referred by key | ||
| * but dont wait for the completion of the increment operation | ||
| * | ||
| * @param key The name of the key | ||
| * @param key The name of the key | ||
| * @param amount The amount to be incremented | ||
| */ | ||
| CompletableFuture<Void> incrCounterAsync(String key, long amount); | ||
|
|
@@ -153,15 +155,15 @@ public interface Context { | |
| /** | ||
| * Update the state value for the key. | ||
| * | ||
| * @param key name of the key | ||
| * @param key name of the key | ||
| * @param value state value of the key | ||
| */ | ||
| void putState(String key, ByteBuffer value); | ||
|
|
||
| /** | ||
| * Update the state value for the key, but don't wait for the operation to be completed | ||
| * | ||
| * @param key name of the key | ||
| * @param key name of the key | ||
| * @param value state value of the key | ||
| */ | ||
| CompletableFuture<Void> putStateAsync(String key, ByteBuffer value); | ||
|
|
@@ -218,19 +220,17 @@ public interface Context { | |
| * Record a user defined metric. | ||
| * | ||
| * @param metricName The name of the metric | ||
| * @param value The value of the metric | ||
| * @param value The value of the metric | ||
| */ | ||
| void recordMetric(String metricName, double value); | ||
|
|
||
| /** | ||
| * Publish an object using serDe for serializing to the topic. | ||
| * Publish an object using serDe or schema class for serializing to the topic. | ||
| * | ||
| * @param topicName | ||
| * The name of the topic for publishing | ||
| * @param object | ||
| * The object that needs to be published | ||
| * @param schemaOrSerdeClassName | ||
| * Either a builtin schema type (eg: "avro", "json", "protobuf") or the class name of the custom schema class | ||
| * @param topicName The name of the topic for publishing | ||
| * @param object The object that needs to be published | ||
| * @param schemaOrSerdeClassName Either a builtin schema type (eg: "avro", "json", "protobuf") or the class name | ||
| * of the custom schema class | ||
| * @return A future that completes when the framework is done publishing the message | ||
| */ | ||
| <O> CompletableFuture<Void> publish(String topicName, O object, String schemaOrSerdeClassName); | ||
|
|
@@ -239,9 +239,30 @@ public interface Context { | |
| * Publish an object to the topic using default schemas. | ||
| * | ||
| * @param topicName The name of the topic for publishing | ||
| * @param object The object that needs to be published | ||
| * @param object The object that needs to be published | ||
| * @return A future that completes when the framework is done publishing the message | ||
| */ | ||
| <O> CompletableFuture<Void> publish(String topicName, O object); | ||
|
|
||
| /** | ||
| * Publish an object using serDe or schema class for serializing to the topic. | ||
| * | ||
| * @param topicName The name of the topic for publishing | ||
| * @param object The object that needs to be published | ||
| * @param schemaOrSerdeClassName Either a builtin schema type (eg: "avro", "json", "protobuf") or the class name | ||
| * of the custom schema class | ||
| * @param messageConf A map of configurations to set for the message that will be published | ||
| * The available options are: | ||
| * | ||
| * "key" - Parition Key | ||
| * "properties" - Map of properties | ||
| * "eventTime" | ||
| * "sequenceId" | ||
| * "replicationClusters" | ||
| * "disableReplication" | ||
| * | ||
| * @return A future that completes when the framework is done publishing the message | ||
| */ | ||
| <O> CompletableFuture<Void> publish(String topicName, O object, String schemaOrSerdeClassName, Map<String, Object> messageConf); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't it better to expose
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we do some caching with schemaOrSerdeClassName so that we don't instantiate the schema/SerDe every single type. And we use schemaOrSerdeClassName as the key for the cache. Not greatest user experience but that is what is happening now |
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| /** | ||
| * 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.functions.api.examples; | ||
|
|
||
| import org.apache.pulsar.client.api.TypedMessageBuilder; | ||
| import org.apache.pulsar.functions.api.Context; | ||
| import org.apache.pulsar.functions.api.Function; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Example function that uses the built in publish function in the context | ||
| * to publish to a desired topic based on config and setting various message configurations to be passed along. | ||
| * | ||
| */ | ||
| public class PublishFunctionWithMessageConf implements Function<String, Void> { | ||
| @Override | ||
| public Void process(String input, Context context) { | ||
| String publishTopic = (String) context.getUserConfigValueOrDefault("publish-topic", "publishtopic"); | ||
| String output = String.format("%s!", input); | ||
|
|
||
| Map<String, String> properties = new HashMap<>(); | ||
| properties.put("input_topic", context.getCurrentRecord().getTopicName().get()); | ||
| properties.putAll(context.getCurrentRecord().getProperties()); | ||
|
|
||
| Map<String, Object> messageConf = new HashMap<>(); | ||
| messageConf.put(TypedMessageBuilder.CONF_PROPERTIES, properties); | ||
| if (context.getCurrentRecord().getKey().isPresent()) { | ||
| messageConf.put(TypedMessageBuilder.CONF_KEY, context.getCurrentRecord().getKey().get()); | ||
| } | ||
| messageConf.put(TypedMessageBuilder.CONF_EVENT_TIME, System.currentTimeMillis()); | ||
| context.publish(publishTopic, output, null, messageConf); | ||
| return null; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| #!/usr/bin/env python | ||
| # | ||
| # 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. | ||
| # | ||
|
|
||
| import time | ||
| from pulsar import Function | ||
|
|
||
| # Example function that uses the built in publish function in the context | ||
| # to publish to a desired topic based on config | ||
| class PublishFunctionWithMessageConf(Function): | ||
| def __init__(self): | ||
| pass | ||
|
|
||
| def process(self, input, context): | ||
| publish_topic = "publishtopic" | ||
| if "publish-topic" in context.get_user_config_map(): | ||
| publish_topic = context.get_user_config_value("publish-topic") | ||
| context.publish(publish_topic, input + '!', | ||
| message_conf={"properties": {k: v for d in [{"input_topic" : context.get_current_message_topic_name()}, context.get_message_properties()] for k, v in d.items()}, | ||
| "partition_key": context.get_partition_key(), | ||
| "event_timestamp": int(time.time())}) | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a python expert. but this PR doesn't seem to specify
partition_keyin the method signature. how does that work?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sijie I have updated the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a ut or integration test for this? this raises a huge concern to me - we update the implementation but didn't change the interface definition. how can we guarantee the fix is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sijie you reviewed the PR when I was in the middle of updating the PR due to discussions and comments around this PR. The original PR did not modify any of the interfaces, but after a discussion with @srkukarni and @merlimat, we decided it was best if we let the user choose what the partition key is. That said, I will add some tests to it
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually @sijie if you look at the commit you commented on. The signature of contextimpl.publish didn't change. Thus context.py did not need to be updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I might be commenting in between commits. but my question is still there : how can we guarantee the fix is correct? where are the tests for this change?