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