thanks for the improvements, +1 On Tue, May 30, 2023 at 2:20 AM Pengcheng Jiang <pengcheng.ji...@streamnative.io.invalid> wrote:
> Hi Mesika: > > Thanks for the suggestions, I updated the pip, and for the rest questions: > > 5. yes, all config goes through arguments instead of a file > 6. it should be a JSON string that can be deserialized to a `Map<String, > Object>`, updated in pip > 7. it should be `pulsar-admin functions localrun` command, updated in pip > 8. the `stateStorageServiceUrl` won't be touched > > Sincerely > Pengcheng Jiang > > Asaf Mesika <asaf.mes...@gmail.com> 于2023年5月29日周一 19:53写道： > > > Hi Pengcheng, > > > > Looks like a solid improvement, definitely helping people using their own > > state store. > > > > I have a few comments: > > > > 1. Background knowledge should explain what is a state storage > > 2. Move problem description from Background Knowledge to Motivation. > > > > I'm quoting the template to understand what should be included in > > the Background knowledge section: > > > > <!-- > > Describes all the knowledge you need to know in order to understand all > the > > other sections in this PIP > > > > * Give a high level explanation on all concepts you will be using > > throughout this document. For example, if you want to talk about > Persistent > > Subscriptions, explain briefly (1 paragraph) what this is. If you're > going > > to talk about Transaction Buffer, explain briefly what this is. > > If you're going to change something specific, then go into more detail > > about it and how it works. > > * Provide links where possible if a person wants to dig deeper into the > > background information. > > > > DON'T > > * Do not include links *instead* explanation. Do provide links for > further > > explanation. > > > > EXAMPLES > > * See [PIP-248](https://github.com/apache/pulsar/issues/19601), > Background > > section to get an understanding on how you add the background knowledge > > needed. > > (They also included the motivation there, but ignore it as we place that > in > > Motivation section explicitly) > > --> > > > > 3. `WorkerConfig` - explain briefly what is Worker and how it differs > from > > Broker. Should be in background knowledge section. > > > > 4. Background knowledge should explain briefly what is a runtime and > > runtime factory. > > > > 5. > > > > Add a new cli argument to JavaInstanceStarter and LocalRunner so > > > process&k8s runtime can pass state related config to them > > > > > > Today all config goes through arguments and not a file? > > > > 6. `--stateStorageConfig` > > What format is the expected value? > > > > 7. `functions local run` > > What is this? > > > > 8. Are you keeping `stateStorageServiceUrl`? Maybe people rely on it? > > > > 9. Don't forget to include link to discussion thread using Apache Pony > Mail > > > > > > On Mon, May 29, 2023 at 10:44 AM Rui Fu <r...@apache.org> wrote: > > > > > Hi Pengcheng, > > > > > > Thanks for bringing this up, the PIP lgtm, +1. > > > > > > Best, > > > > > > Rui Fu > > > On May 29, 2023 at 13:52 +0800, Enrico Olivelli <eolive...@gmail.com>, > > > wrote: > > > > Looks good > > > > +1 > > > > > > > > Enrico > > > > > > > > Il Lun 29 Mag 2023, 04:47 Pengcheng Jiang > > > > <pengcheng.ji...@streamnative.io.invalid> ha scritto: > > > > > > > > > Dear Pulsar community, > > > > > > > > > > I created a pip to make pulsar functions' `StateStoreProvider` > > > configurable > > > > > with custom configurations: > > > https://github.com/apache/pulsar/issues/20419 > > > > > > > > > > Any feedback and suggestions are welcome > > > > > > > > > > Sincerely > > > > > Pengcheng Jiang > > > > > > > > > > >
