asafm commented on code in PR #20455: URL: https://github.com/apache/pulsar/pull/20455#discussion_r1220994141
########## pip/pip-272.md: ########## @@ -42,11 +15,11 @@ Currently, Pulsar uses Bookkeeper as the default state storage interface. We can The `WorkerConfig` is used to configure the Pulsar functions worker and has two fields related to the state store: -1. `stateStorageProviderImplementation`: The implementation class for the state store which should implement the interface`StateStoreProvider`, such as `org.apache.pulsar.functions.instance.state.BKStateStoreProviderImpl` +1. `stateStorageProviderImplementation`: The implementation class for the state store which should implement the interface `StateStoreProvider`, such as `org.apache.pulsar.functions.instance.state.BKStateStoreProviderImpl` 2. `stateStorageServiceUrl`: The service URL of state storage, such as: `bk://localhost:4181` -`Runtime` and `RuntimeFactory`: +## `Runtime` and `RuntimeFactory`: Review Comment: ```suggestion ## `Runtime` and `RuntimeFactory` ``` ########## pip/pip-272.md: ########## @@ -59,91 +32,41 @@ Pulsar Function supports three kinds of runtime and, correspondingly, has three # Motivation -<!-- -Describe the problem this proposal is trying to solve. - -* Explain what is the problem you're trying to solve - current situation. -* This section is the "Why" of your proposal. ---> - Pulsar functions don't support configuring the `StateStoreProviderImplClass` with extra configurations, although there is a `Map<String, Object> config` parameter in the `StateStoreProvider#init` method, it's initialized as a map with only one field `stateStorageServiceUrl` in `javaInstanceRunnable` see: -https://github.com/apache/pulsar/blob/master/pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceRunnable.java#L366-L369 +https://github.com/apache/pulsar/blob/0a39b819a9ec371322bfde61685497a2c21d9ab3/pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceRunnable.java#L366-L369 Then when using a custom `StateStoreProvider`, it cannot get other configurations from the `init` method. # Goals ## In Scope -<!-- -What this PIP intend to achieve once It's integrated into Pulsar. -Why does it benefit Pulsar. ---> - Make `StateStoreProvider` "truly" configurable, custom `StateStoreProvider` can be configured more easily and explicitly. ## Out of Scope -<!-- -Describe what you have decided to keep out of scope, perhaps left for a different PIP/s. ---> - # High Level Design -<!-- -Describe the design of your solution in *high level*. -Describe the solution end to end, from a birds-eye view. -Don't go into implementation details in this section. - -I should be able to finish reading from beginning of the PIP to here (including) and understand the feature and -how you intend to solve it, end to end. - -DON'T -* Avoid code snippets, unless it's essential to explain your intent. ---> - -Make `StateStoreProvider` configurable using custom configurations. +We will add a configuration key to the Pulsar Function Worker configuration, containing the configuration for the `StateStoreProvider` implementation as a `Map<String, Object>`. Review Comment: I think people can't relate `map<String, object>` to the configuration of Pulsar Function Worker. Map<string, object> is Java talk. As I replied in the comment, we need to fix this somehow. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
