Re: My feedback on PR 320, and reflections on the process

I am sorry that I have not had time to respond to this earlier.

>>  logical operator, we think a more correct word is comparison-operator

Absolutely. Will be fixed

>> The second is fundamental which is to not modify subscription interval
with the query parameter.
 In order to support my opinion I suggested that subscription is not
supported on REST anyway and therefore there's no reason to encode this  in
the URL.

The latter is a quite valid point.
If we remove $interval from the query, then we must be consistent and also
remove $range and $change, all of them will then appear in the Websocket
subscribe payload instead.
I will do that.

>>  The third is a spelling error.

Where do I find it?

>> Should the system really return *every* item below it, or might there be
some other interesting information we want to get returned about the branch
node itself only?

I assume you are referring to a service discovery request, as a branch node
else have nothing to return? If so, the client has the means to control
this as the query is $spec=depth, where depth is how deep into the subtree
data will be fetched for the response. With depth set to 1 only the direct
children of the root pointed to by the path will be returned.
So for service discovery requests I do not see a need to have a wildcard at
the end of the path as it is always implicitly understood to be there.
For Read requests I think it makes sense to require that a wildcard is put
at the end of the path if the request is to return data from all leaf nodes
in the underlying subtree.

>> The fifth looks like a copy-paste error.

Where do I find it?

>> The eighth is a detail, just the naming of a word.

Where do I find it?

Please help me to pint to the ones I cannot figure out where to find, and I
will fix them and the other I mention above.
Thanks for good feedback.

As we have agreed on, it is fully acceptable to come with further comments,
change proposals also after a PR is merged.

BR
Ulf

On Fri, Dec 13, 2019 at 1:13 PM Gunnar Andersson <gandersson@genivi.org>
wrote:

> On Thu, 2019-12-12 at 10:57 -0500, Ted Guild wrote:
> > I think you got the wrong Ulf, adding the correct one. Not surprising
> > that you know more than one in Sweden :) I left Ulf Wiger in bcc so
> > he
> > knows we found the right one.
> >
> > A small comment to agree with Gunnar's second point, subscription on
> > REST doesn't make sense unless it returns or redirects to WebSocket.
> >
> > Regarding the fourth point, that is the inherit risk of queries -
> > getting too broad a result set back.
>
> Of course that could be seen as an inherit risk, but knowing that we
> can still design the interface as well as we can, and should therefore
> discuss these details carefully.  The point was that the spec specifies
> that if you ask for a branch node, like X.Y.Z you get everything below
> it, but the same could be achieved by asking for X.Y.Z.* instead.
>
> I think the latter is more future proof in a way.
> For example if in one version of VSS:
>  X.Y.Z and Z is a leaf then you get one node and that is what you
> expect, but if in a future version Z now has child-nodes
> X.Y.Z.P.Q so those start getting returned unexpectedly.
> I think if there is an explicit wildcard, then you're more expecting
> this risk, that the returned set can be a bit unexpected.
> And you would never use a wildcard if what you think you are addressing
> is a leaf node?
>
> > The difference here is the service
> > may also be restricting what signals the client application is
> > permitted to see further than the filter does.
>
> Yes, that's also true, but independent of the detail I brought up.
>
> - Gunnar
>
> >
> > On Tue, 2019-12-10 at 21:07 +0000, Gunnar Andersson wrote:
> > > Hi guys,
> > >
> > > Warning, wall of text incoming...
> > >
> > > I looked back at my feedback and I'm not sure "pointing to the
> > > mistakes" handles the big picture so I do also that here.
> > > Only perhaps 2 could be considered simple mistakes in my opinion,
> > > and
> > > then there is a range of things that include preferences, opinions,
> > > but
> > > also a few fundamental aspects.
> > >
> > > So I end up mostly repeating my feedback here, I suppose you can
> > > then
> > > do what you wish with it with each of the points.
> > >
> > > The first feedback was from me and Patrick identically.  We think
> > > you
> > > made a mistake in writing logical operator, we think a more correct
> > > word is comparison-operator
> > >
> > >
> >
> https://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B#Logical_operators
> > > The second is fundamental which is to not modify subscription
> > > interval
> > > with the query parameter.  I simply express that I am opposed to
> > > this
> > > and therefore something I thought would be worth discussing and
> > > resolving, but maybe we have to come back to it then?
> > > I felt that if we only remove this usage of the query string, then
> > > the
> > > other usage of the query string is for querying/filtering of the
> > > selected data, which is consistent usage.  You often speak about
> > > the
> > > importance of consistent and good designs, Ulf.  Please consider
> > > it.
> > >
> > > In order to support my opinion I suggested that subscription is not
> > > supported on REST anyway and therefore there's no reason to encode
> > > this
> > > in the URL.  The WebSocket variation does this as a parameter
> > > inside
> > > the JSON request (which is also defined in VISS and therefore
> > > backward
> > > compatible as a bonus).
> > >
> > > I then asked if it is true and agreed by all, that there is no
> > > subscription on REST (also I asked you to confirm that there is
> > > none
> > > shown in the demo?)  Can we find group consensus on that, or
> > > otherwise
> > > discuss how it is supposed to work.
> > >
> > > The third is a spelling error.  There could be a quick fix before
> > > merging.  More about consequences of the process later.
> > >
> > > The fourth is quite fundamental behavior of the system beyond
> > > queries
> > > I
> > > think and we should be careful that it does not just sneak in.
> > > What happens when you address a branch node in isolation?  Should
> > > the
> > > system really return *every* item below it, or might there be some
> > > other interesting information we want to get returned about the
> > > branch
> > > node itself only?  An alternative to this behavior is a system that
> > > requires explicitly using a wildcard query if the user wants
> > > "everything below", which feels a bit better/safer to me.
> > >
> > > It's worth discussing the pros and cons about something like
> > > that.  We
> > > can do that after or before merging, but I think it's fundamental
> > > to
> > > the concept?
> > >
> > > (Some of these things not being discussed lead me to feel this is
> > > more
> > > like a full concept coming from Ulf only, without the time to get
> > > consensus on that concept.  The core problem there is however that
> > > I
> > > was not aware and did not start commenting before last week, so
> > > it's
> > > perhaps my fault of not starting in time with this process.  But it
> > > doesn't explain why it's suddenly merged this week because another
> > > week
> > > has passed.  Because nothing really new happened this week - my
> > > feedback was given right after the last meeting, and then just left
> > > open)
> > >
> > > Overall I feel like I may have have missed the introductory
> > > discussion
> > > to this whole query concept before a PR was sent?  But proposing
> > > fundamental new parts of the spec using PRs is also a way of
> > > working
> > > that can be good.   It can be very concrete.   But then I really
> > > think
> > > big discussions about the concept need to be allowed on the PR, and
> > > a
> > > bit longer time allowed for the concept itself to mature in
> > > discussions.
> > >
> > > The fifth looks like a copy-paste error.   Mistakes found in
> > > reviews
> > > normally gets fixed quickly and updated in a PR before it is
> > > merged.
> > > If we do not do that, then we get this:
> > >
> > >   1) Some of the time spent reviewing is completely wasted time
> > >   2) We might forget(!) fixing it, despite that it was identified
> > > and
> > > found.
> > >   3) Fixing it later adds additional "useless" commits to the
> > > history
> > >
> > > (A useful commit history is a whole other ball of wax which I
> > > almost
> > > not dare to raise but that is one consequence anyway).
> > >
> > > The sixth was about a preference, how to name $word and making sure
> > > we
> > > align to the normal web conventions.  This could be considered a
> > > detail
> > > of course.  But I think it ought to be possible to organize and
> > > drive
> > > an (efficient) decision process also on minor details.
> > >
> > > Then comes some quite fundamental feedback from me behind the
> > > philosophy of this proposal.  Again I wanted some opportunity to
> > > find
> > > an understanding between myself and Ulf, and others, perhaps
> > > brainstorm
> > > from that discussion to out if we can together reach something even
> > > better!  As long as we stay away from "data model" I think and hope
> > > such discussions might be relatively efficient even in group
> > > discussion, if they are moderated.
> > >
> > > The eighth is a detail, just the naming of a word.  Again, normal
> > > process however, I would expect it is resolved by getting feedback
> > > on
> > > the feedback, so that at least it's visible that the feedback was
> > > received, even if it is not accepted.
> > >
> > > Then comes the main syntax discussion about the non-standard
> > > EQ/LT/GT
> > > vs =, <, and >.  Of course that could be considered a detail that
> > > is
> > > changed in a future PR - with the consequences I already mentioned
> > > about that process.
> > >
> > > So then, about the process, I think it is generally good that we
> > > have
> > > a
> > > time limit and push forward.  The 2-week limit process we have was
> > > as
> > > I
> > > understand primarily to drive people to review and give feedback,
> > > and
> > > if that does not happen in time then they run the risk that things
> > > just
> > > get merged without their input.   In other words, "React within 2
> > > weeks
> > > or you have to accept the consequences".  This can be a good
> > > incentivizer.
> > >
> > > But when someone actually takes the time to give feedback (I was
> > > late
> > > on this one, but as of today my feedback has been there for 1
> > > week),
> > > then I would think that this also needs to be addressed?  Otherwise
> > > shouldn't the opposite should apply - that the submitter risks it
> > > not
> > > being merged, because the discussion stopped from their end?  There
> > > needs to be an incentivized drive from all ends.
> > >
> > > If the GitHub discussion has failed for some time to drive towards
> > > a
> > > decision on details of a proposal, such as exact naming of words,
> > > then
> > > at minimum I propose that we organize and review all those open
> > > questions on the call and say "Knowing all of this, shall we merge
> > > anyway?".  Be explicit about it.  Just put those things up as a
> > > list
> > > of
> > > open question and get a quick majority vote on the call?  This I
> > > feel
> > > this is more a failing process in simple organization of all the
> > > minor
> > > issues we do not have consensus on, rather than the problem is
> > > "we're
> > > not merging fast enough".
> > >
> > > So, with that said, I guess today's decision was to leave it in
> > > Ulf's
> > > hands to fix what he wants from the above feedback, and then the PR
> > > is
> > > merged, with further improvements possible.   Will we, at some
> > > point,
> > > have the fundamental discussion about the philosophy behind this
> > > query
> > > language or is this already done from the group's point of
> > > view?  Last
> > > week had mostly time for "here is the proposal and here is the
> > > demo"
> > > which was great to get the understanding.  But then we did really
> > > not
> > > have the opportunity for the reaction.  (I did gave some feedback
> > > on
> > > the call but we agreed that I need to write it down, which I did
> > > immediately).
> > >
> > > - Gunnar
> > >
> > >
> > >
> > >
>
>

-- 
Ulf Bjorkengren
*Geotab*
Senior Connectivity Strategist | Ph. D.
Mobile +45 53562142
Visit www.geotab.com

Join us at Connect 2020

San Diego

January 13 - 16, 2020

Register Now! <https://www.geotab.com/connect/>