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 > > > > > > >
