<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Just a reminder: are there any more suggestions/remarks<br>
before this feature can be applied?<br>
<br>
<br>
Eduard.<br>
<br>
<br>
<div class="moz-cite-prefix">On 26.03.2017 00:04, Eduard Bagdasaryan
wrote:<br>
</div>
<blockquote
cite="mid:8f5dde69-e3f1-03a3-bc88-4e24213f1320@measurement-factory.com"
type="cite">Adjusted the patch accordingly.
<br>
<br>
<br>
Eduard.
<br>
<br>
<br>
On 22.03.2017 15:58, Amos Jeffries wrote:
<br>
<blockquote type="cite">On 17/03/2017 2:11 a.m., Eduard
Bagdasaryan wrote:
<br>
<blockquote type="cite">On 16.03.2017 10:15, Amos Jeffries
wrote:
<br>
<br>
<blockquote type="cite">in src/neighbours.cc:
<br>
<br>
* peerConnectTimeout() should be a member of the CachePeer
class yes?
<br>
</blockquote>
The initial patch version did as you suggest, but then we
decided
<br>
to use separate method instead, to avoid 'heavy' dependency on
<br>
SquidConfig.h (which periodically causes linkage problems,
IIRC).
<br>
<br>
</blockquote>
Hmm. Okay for now, but please add a TODO or XXX about that to
the function.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">in src/tunnel.cc:
<br>
<br>
* "start" is an action name and we use it (almost?)
exclusively for Job
<br>
initiation. By comparison "started" means/implies more
clearly a state
<br>
or time point. The Tunnel member stores a time point.
<br>
- IMO both are bad, but "started" is better than "start".
Better still
<br>
would be a name that actually describes *what* has been
start(ed).
<br>
</blockquote>
I do not mind renaming it, e.g., into 'creationTime'.
<br>
<br>
</blockquote>
Sounds good to me.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">in src/PeerPoolMgr.cc:
<br>
<br>
* in PeerPoolMgr::handleOpenedConnection() I see a line
doing max(1,
<br>
(peerTimeout - timeUsed)); just like the code in FwdState.cc
<br>
FwdState::connectDone().
<br>
BUT, this PoolMgr is (wrongly) using int instead of
time_t. Perhapse
<br>
that should be changed and something de-duplicate that
max(1, x) usage?
<br>
</blockquote>
I am not against changing int to time_t there. As for 'max',
how about
<br>
creating inside neighbors.cc a helper method:
<br>
time_t PositiveTimeout(const time_t timeout) { return max(1,
timeout); }
<br>
<br>
</blockquote>
Sounds good. I did a few tests to see if we could also avoid the
<br>
static_cast, but it seems not in any simple way. So dont forget
that in
<br>
the new function.
<br>
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">in src/comm/Connection.cc:
<br>
<br>
* I dont see why EnoughTimeToReForward() should be in the
Comm::
<br>
namespace. It is about messageing layer timing. So is more
appropriate
<br>
to be in FwdState:: or maybe SquidTime.h/time.cc.
<br>
- if you pick time.cc that would also mean the fairly
generic time_t
<br>
static functions it calls would move to time.cc where they
seem more
<br>
appropriate.
<br>
</blockquote>
Again, I am not against moving this method and
ForwardTimeout() into
<br>
FwdState.cc (keeping related '*Forward' methods there makes
some sense).
<br>
<br>
</blockquote>
Okay, I can live with that.
<br>
<br>
Amos
<br>
<br>
</blockquote>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
squid-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:squid-dev@lists.squid-cache.org">squid-dev@lists.squid-cache.org</a>
<a class="moz-txt-link-freetext" href="http://lists.squid-cache.org/listinfo/squid-dev">http://lists.squid-cache.org/listinfo/squid-dev</a>
</pre>
</blockquote>
<br>
</body>
</html>