Re: svn commit: r1803899 - in /subversion/trunk/subversion: libsvn_ra_serf/ libsvn_subr/ mod_dav_svn/

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803899 - in /subversion/trunk/subversion: libsvn_ra_serf/ libsvn_subr/ mod_dav_svn/

Stefan Sperling
On Wed, Aug 02, 2017 at 06:49:55PM -0000, [hidden email] wrote:
> +svn_boolean_t
> +svn_ra_serf__is_local_network(svn_ra_serf__session_t *session)
> +{
> +  return session->conn_latency >= 0 &&
> +         session->conn_latency < apr_time_from_msec(5);
> +}

"local network" is a rather blurry concept.
This will often return FALSE for local wireless connections, for
example, because of shared medium contention.

I agree that 5msec is a well chosen latency threshold.
But why not name this function after the question it answers,
e.g. svn_ra_serf__is_low_latency_connection()?
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803899 - in /subversion/trunk/subversion: libsvn_ra_serf/ libsvn_subr/ mod_dav_svn/

Julian Foad-5
Stefan Sperling wrote:

> On Wed, Aug 02, 2017 at 06:49:55PM -0000, [hidden email] wrote:
>> +svn_boolean_t
>> +svn_ra_serf__is_local_network(svn_ra_serf__session_t *session)
>> +{
>> +  return session->conn_latency >= 0 &&
>> +         session->conn_latency < apr_time_from_msec(5);
>> +}
>
> "local network" is a rather blurry concept.
> [...] why not name this function after the question it answers,
> e.g. svn_ra_serf__is_low_latency_connection()?

Am I right in thinking what the caller really wants to know is whether
this is a high-bandwidth connection? If so, this is a case of the
following pattern:

  * the caller wants to know one thing (is it high bandwidth),
  * the available information is something else (is it low latency),
  * the caller uses the available information to make a guess.

Naming the helper function 'is_local' splits the guesswork to two
places, the caller and the callee. It really requires comments at both
places, otherwise code that does not do what it says can become
confusing later.

Naming the helper function 'is_low_latency' would make this function do
exactly what its name says; self-documenting is a good thing. A comment
at the point of call should then explain that it is using 'low latency'
to make a guess about 'high bandwidth'.

That is probably better, so long as there is only one such caller.

A self-documenting option that scales to multiple callers is to put all
the guesswork in the helper function, and name it so as to recognize that:

is_probably_high_bandwidth() {
  /* the best we can do for now is to guess from the latency */
  return (latency < X);
}

That is probably best.

- Julian
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803899 - in /subversion/trunk/subversion: libsvn_ra_serf/ libsvn_subr/ mod_dav_svn/

Evgeny Kotkov
In reply to this post by Stefan Sperling
Stefan Sperling <[hidden email]> writes:

> "local network" is a rather blurry concept.
> This will often return FALSE for local wireless connections, for
> example, because of shared medium contention.
>
> I agree that 5msec is a well chosen latency threshold.
> But why not name this function after the question it answers,
> e.g. svn_ra_serf__is_low_latency_connection()?

My original intention here was to have a function that can distinguish
between LAN and WAN, using the information that we have — that's
latency now and possibly something else in the future.  But indeed,
considering local wireless connections, the name of the function could
be misleading.

Something like svn_ra_serf__is_low_latency_connection() would probably
be more appropriate right now.  And, we could deal with the additional
information about the connection to base the detection on, if and when
we have that.

I'll try to update the function name and the related comments.


Thanks,
Evgeny Kotkov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803899 - in /subversion/trunk/subversion: libsvn_ra_serf/ libsvn_subr/ mod_dav_svn/

Evgeny Kotkov
Evgeny Kotkov <[hidden email]> writes:

> Stefan Sperling <[hidden email]> writes:
>
>> "local network" is a rather blurry concept.
>> This will often return FALSE for local wireless connections, for
>> example, because of shared medium contention.
>>
>> I agree that 5msec is a well chosen latency threshold.
>> But why not name this function after the question it answers,
>> e.g. svn_ra_serf__is_low_latency_connection()?
>
> My original intention here was to have a function that can distinguish
> between LAN and WAN, using the information that we have — that's
> latency now and possibly something else in the future.  But indeed,
> considering local wireless connections, the name of the function could
> be misleading.
>
> Something like svn_ra_serf__is_low_latency_connection() would probably
> be more appropriate right now.  And, we could deal with the additional
> information about the connection to base the detection on, if and when
> we have that.
>
> I'll try to update the function name and the related comments.

Committed in https://svn.apache.org/r1803989


Regards,
Evgeny Kotkov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803899 - in /subversion/trunk/subversion: libsvn_ra_serf/ libsvn_subr/ mod_dav_svn/

Branko Čibej
In reply to this post by Evgeny Kotkov
On 03.08.2017 13:53, Evgeny Kotkov wrote:

> Stefan Sperling <[hidden email]> writes:
>
>> "local network" is a rather blurry concept.
>> This will often return FALSE for local wireless connections, for
>> example, because of shared medium contention.
>>
>> I agree that 5msec is a well chosen latency threshold.
>> But why not name this function after the question it answers,
>> e.g. svn_ra_serf__is_low_latency_connection()?
> My original intention here was to have a function that can distinguish
> between LAN and WAN, using the information that we have — that's
> latency now and possibly something else in the future.  But indeed,
> considering local wireless connections, the name of the function could
> be misleading.
>
> Something like svn_ra_serf__is_low_latency_connection() would probably
> be more appropriate right now.  And, we could deal with the additional
> information about the connection to base the detection on, if and when
> we have that.
>
> I'll try to update the function name and the related comments.


It occurs to me that latency on a 10 mbps wired ethernet LAN will be
about the same as on a 1000 mbps wired ethernet LAN, especially if the
packets used to measure latency are small. ... but the bandwidth will be
wildly different.

(Just throwing this into the mix because I can't actually imagine anyone
using a 10 mbps ethernet in a development environment these days, but
who knows ...)

-- Brane

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803899 - in /subversion/trunk/subversion: libsvn_ra_serf/ libsvn_subr/ mod_dav_svn/

Daniel Shahaf-2
Branko Čibej wrote on Fri, Aug 04, 2017 at 13:54:04 +0200:
> It occurs to me that latency on a 10 mbps wired ethernet LAN will be
> about the same as on a 1000 mbps wired ethernet LAN, especially if the
> packets used to measure latency are small. ... but the bandwidth will be
> wildly different.
>
> (Just throwing this into the mix because I can't actually imagine anyone
> using a 10 mbps ethernet in a development environment these days, but
> who knows ...)

The argument is equally sound for 100Mbps v. 10Gbps.
Loading...