On Tue, Apr 5, 2022 at 12:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Apr 5, 2022 at 6:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Here are my comments for the latest patch v6-0001.
> >
> > (I will post my v6-0002 review comments separately)
> >
> > PATCH v6-0001 comments
> > ======================
> >
> > 1.1 General - Option name
> >
> > I still feel like the option name is not ideal. Unfortunately, this is
> > important because any name change would impact lots of these patch
> > files and docs, struct members etc.
> >
> > It was originally called "local_only", but I thought that as a
> > SUBSCRIPTION option that was confusing because "local" means local to
> > what? Really it is local to the publisher, not local to the
> > subscriber, so that name seemed misleading.
> >
> > Then I suggested "publish_local_only". Although that resolved the
> > ambiguity problem, other people thought it seemed odd to have the
> > "publish" prefix for a subscription-side option.
> >
> > So now it is changed again to "subscribe_local_only" -- It's getting
> > better but still, it is implied that the "local" means local to the
> > publisher except there is nothing in the option name really to convey
> > that meaning. IMO we here all understand the meaning of this option
> > mostly by familiarity with this discussion thread, but I think a user
> > coming to this for the first time will still be confused by the name.
> >
> > Below are some other name ideas. Maybe they are not improvements, but
> > it might help other people to come up with something better.
> >
> > subscribe_publocal_only = true/false
> > origin = pub_only/any
> > adjacent_data_only = true/false
> > direct_subscriptions_only = true/false
> > ...
> >
>
> I think local_only with a description should be okay considering other
> current options. Some of the options like slot_name, binary, etc. are
> publisher-specific which becomes clear only after reading the
> description. I am not sure adding pub*/sub* to it is much helpful. So,
> +1 to a simple boolean like 'local_only', 'data_local', or something
> like that.
+1 to use a simple boolean option.
Thanks for the examples of the other options which are also really
publisher-specific. Seen in that light, and with a clear description,
probably the original "local_only" was fine after all.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
