On 08/25/2016 04:04 AM, Eduard Bagdasaryan wrote: > Therefore, we could use the timestamp if Last-Modified is unavailable.
I do not understand why the patch hides the lastmod field behind a basic getter. If we assert that a timestamp-based last modification value should be used in many cases, then we need to force the calling code to make an important choice between a raw lastmod and a timestamp-based last modification value. The patch does not force the caller to make that choice because developers will naturally call lastModified() getter instead of effectiveModificationTime(). If calling lastModified() produces more problems like the one you are trying to fix (with reviewers not being able to guess that the wrong method was called), then renaming lastmod_ accomplished nothing. Similarly, if calling lastModified() produces no new problems (because the lastmod usage case you were fixing happened to be exceptional rather than the norm), then renaming lastmod_ also accomplished nothing. The patch does use effectiveModificationTime() in several places already so I suspect its usage is not that exceptional -- many, possibly even most contexts should use effectiveModificationTime(). If you agree, then we should make using lastmod_ getter more difficult -- the developer and the reviewer should be forced to pay attention to such use because it is likely to indicate a bug in new code (i.e., the new code should have called effectiveModificationTime() instead). Moreover, these raw lastmod abuse problems might be already surfacing in your own patch! I see several raw lastmod value uses there: 1. Misleading debugging: > + const time_t lastmod = http->storeEntry()->effectiveModificationTime(); > + http->request->lastmod = lastmod; ... > - debugs(88, 5, "clientReplyContext::processExpired : lastmod " << > entry->lastmod ); > + debugs(88, 5, "lastmod " << entry->lastModified()); In other words, Squid is going to use the effective modification time, but we are logging raw time to make triage harder. 2. Writing -1 to cache store metadata: > currentEntry(sd->addDiskRestore(key, > tmpe.timestamp, ... > - tmpe.lastmod, > + tmpe.lastModified(), and > StoreSwapLogData s; > s.timestamp = e.timestamp; ... > - s.lastmod = e.lastmod; > + s.lastmod = e.lastModified(); and > - basics.lastmod = from.lastmod; > + basics.lastmod = from.lastModified(); Does a store need raw lastmod or effective modification time? I suspect it is the latter, but perhaps I am missing some valid use cases for the former. This needs a careful consideration. If we are lucky, there is a relatively simple way to answer this question: If the stored lastmod value is only used to re-initialize some future StoreEntry::lastmod_ value _and_ all non-debugging uses of StoreEntry::lastmod_ are going to be via effectiveModificationTime(), then we can store effective modification time instead of -1! 3. Sending an HTCP message to another service. > - hdr.putTime(Http::HdrType::LAST_MODIFIED, e->lastmod); > + if (e && e->lastModified() > -1) > + hdr.putTime(Http::HdrType::LAST_MODIFIED, e->lastModified()); Is this a conditional/revalidation request? If yes, then should we use an effective modification time instead, like we do in use case #1? 4. StoreEntry state reporting. > describeTimestamps(const StoreEntry * entry) ... > (int) entry->timestamp, ... > - (int) entry->lastmod, > + (int) entry->lastModified(), The debugging should reflect the true value so this is fine. However, we can make this function a StoreEntry method to avoid the need for getter _if_ this is the only place where the getter is needed. If I did not miss any use cases, then #3 appears to be critical here. If we should be using effective modification time in #3, then there appears to be no need to store -1 in StoreEntry and your patch can be greatly simplified. If #3 does need a raw value from StoreEntry, then we will need a better way to force developers to think about the right method to use. Please double check what is going on in #3, and we will go from there. Thank you, Alex. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev