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

Ulf,

Thanks for looking at this.  All of this email was basically repetition of the review comments I  already made on the PR.  I only wanted to show the diversity of things that were open (from my perspective). 

You can find the spelling error and minor things there in GitHub, and probably also more details on the other major points, and of course the comments there point out which line number they are for.

Hope this helps,

--
Gunnar Andersson <gandersson@genivi.org>
Development Lead, GENIVI Alliance

--------------------------------
From: Ulf Bjorkengren <ulfbjorkengren@geotab.com>
Sent: Friday, December 13, 2019 16:04
To: Gunnar Andersson; public-automotive
Cc: ted@w3.org; Peter Winzell
Subject: 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 <mailto: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!