You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by bc...@apache.org on 2018/08/31 19:11:34 UTC

[trafficserver] 01/02: Make negative caching accept configured error status codes

This is an automated email from the ASF dual-hosted git repository.

bcall pushed a commit to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git

commit 0a54f31dfc718810836baa71edb4e7f9a9041bd0
Author: Xavier Chi <ch...@gmail.com>
AuthorDate: Mon Jul 16 15:51:23 2018 -0500

    Make negative caching accept configured error status codes
    
    (cherry picked from commit 4c34cbc4d0ad432b56b2f61f18f63a2d41cc94b8)
    
     Conflicts:
    	proxy/http/HttpConfig.h
---
 doc/admin-guide/files/records.config.en.rst |  9 ++++-
 doc/admin-guide/performance/index.en.rst    |  4 ++-
 lib/perl/lib/Apache/TS/AdminClient.pm       |  1 +
 mgmt/RecordsConfig.cc                       |  2 ++
 proxy/http/HttpConfig.cc                    | 54 +++++++++++++++++++++++++++++
 proxy/http/HttpConfig.h                     |  6 ++++
 proxy/http/HttpTransact.cc                  | 23 ++++--------
 7 files changed, 81 insertions(+), 18 deletions(-)

diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst
index a2f0ee3..f8d51a5 100644
--- a/doc/admin-guide/files/records.config.en.rst
+++ b/doc/admin-guide/files/records.config.en.rst
@@ -1468,7 +1468,7 @@ Negative Response Caching
    When disabled (``0``), |TS| will only cache the response if the response has
    ``Cache-Control`` headers.
 
-   The following negative responses are cached by Traffic Server:
+   The following negative responses are cached by Traffic Server by default:
 
    ====================== =====================================================
    HTTP Response Code     Description
@@ -1478,6 +1478,7 @@ Negative Response Caching
    ``400``                Bad Request
    ``403``                Forbidden
    ``404``                Not Found
+   ``414``                URI Too Long
    ``405``                Method Not Allowed
    ``500``                Internal Server Error
    ``501``                Not Implemented
@@ -1496,6 +1497,12 @@ Negative Response Caching
    How long (in seconds) Traffic Server keeps the negative responses  valid in cache. This value only affects negative
    responses that do NOT have explicit ``Expires:`` or ``Cache-Control:`` lifetimes set by the server.
 
+.. ts:cv:: CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 405 414 500 501 502 503 504
+   :reloadable:
+
+   The HTTP status code for negative caching. Default values are mentioned above. The unwanted status codes can be
+   taken out from the list. Other status codes can be added. The variable is a list but parsed as STRING.
+
 .. ts:cv:: CONFIG proxy.config.http.negative_revalidating_enabled INT 0
    :reloadable:
    :overridable:
diff --git a/doc/admin-guide/performance/index.en.rst b/doc/admin-guide/performance/index.en.rst
index 86c3b6e..18f05dc 100644
--- a/doc/admin-guide/performance/index.en.rst
+++ b/doc/admin-guide/performance/index.en.rst
@@ -497,10 +497,12 @@ client request to result in an origin request.
 
 This behavior is controlled by both enabling the feature via
 :ts:cv:`proxy.config.http.negative_caching_enabled` and setting the cache time
-(in seconds) with :ts:cv:`proxy.config.http.negative_caching_lifetime`. ::
+(in seconds) with :ts:cv:`proxy.config.http.negative_caching_lifetime`. Configured
+status code for negative caching can be set with :ts:cv:`proxy.config.http.negative_caching_list`. ::
 
     CONFIG proxy.config.http.negative_caching_enabled INT 1
     CONFIG proxy.config.http.negative_caching_lifetime INT 10
+    CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 405 414 500 501 502 503 504
 
 SSL-Specific Options
 ~~~~~~~~~~~~~~~~~~~~
diff --git a/lib/perl/lib/Apache/TS/AdminClient.pm b/lib/perl/lib/Apache/TS/AdminClient.pm
index 0f52151..344acec 100644
--- a/lib/perl/lib/Apache/TS/AdminClient.pm
+++ b/lib/perl/lib/Apache/TS/AdminClient.pm
@@ -475,6 +475,7 @@ The Apache Traffic Server Administration Manual will explain what these strings
  proxy.config.http.keep_alive_no_activity_timeout_out
  proxy.config.http.keep_alive_post_out
  proxy.config.http.negative_caching_enabled
+ proxy.config.http.negative_caching_list
  proxy.config.http.negative_caching_lifetime
  proxy.config.http.negative_revalidating_enabled
  proxy.config.http.negative_revalidating_lifetime
diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc
index 8ea0000..ac4cd3b 100644
--- a/mgmt/RecordsConfig.cc
+++ b/mgmt/RecordsConfig.cc
@@ -499,6 +499,8 @@ static const RecordElement RecordsConfig[] =
   ,
   {RECT_CONFIG, "proxy.config.http.negative_caching_lifetime", RECD_INT, "1800", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
   ,
+  {RECT_CONFIG, "proxy.config.http.negative_caching_list", RECD_STRING, "204 305 403 404 405 414 500 501 502 503 504", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
+  ,
 
   //        #########################
   //        # proxy users variables #
diff --git a/proxy/http/HttpConfig.cc b/proxy/http/HttpConfig.cc
index 7aa54e8..8e40cbc 100644
--- a/proxy/http/HttpConfig.cc
+++ b/proxy/http/HttpConfig.cc
@@ -900,6 +900,56 @@ register_stat_callbacks()
                      (int)http_sm_finish_time_stat, RecRawStatSyncSum);
 }
 
+static bool
+set_negative_caching_list(const char *name, RecDataT dtype, RecData data, HttpConfigParams *c, bool update)
+{
+  bool ret = false;
+  HttpStatusBitset set;
+  // values from proxy.config.http.negative_caching_list
+  if (0 == strcasecmp("proxy.config.http.negative_caching_list", name) && RECD_STRING == dtype && data.rec_string) {
+    // parse the list of status code
+    ts::TextView status_list(data.rec_string, strlen(data.rec_string));
+    auto is_sep{[](char c) { return isspace(c) || ',' == c || ';' == c; }};
+    while (!status_list.ltrim_if(is_sep).empty()) {
+      ts::TextView span, token{status_list.take_prefix_if(is_sep)};
+      auto n = ts::svtoi(token, &span);
+      if (span.size() != token.size()) {
+        Error("Invalid status code '%.*s' for negative caching: not a number", static_cast<int>(token.size()), token.data());
+      } else if (n <= 0 || n >= HTTP_STATUS_NUMBER) {
+        Error("Invalid status code '%.*s' for negative caching: out of range", static_cast<int>(token.size()), token.data());
+      } else {
+        set[n] = 1;
+      }
+    }
+  }
+  // set the return value
+  if (set != c->negative_caching_list) {
+    c->negative_caching_list = set;
+    ret                      = ret || update;
+  }
+  return ret;
+}
+
+// Method of getting the status code bitset
+static int
+negative_caching_list_cb(const char *name, RecDataT dtype, RecData data, void *cookie)
+{
+  HttpConfigParams *c = static_cast<HttpConfigParams *>(cookie);
+  // Signal an update if valid value arrived.
+  if (set_negative_caching_list(name, dtype, data, c, true)) {
+    http_config_cb(name, dtype, data, cookie);
+  }
+  return REC_ERR_OKAY;
+}
+
+// Method of loading the negative caching config bitset
+void
+load_negative_caching_var(RecRecord const *r, void *cookie)
+{
+  HttpConfigParams *c = static_cast<HttpConfigParams *>(cookie);
+  set_negative_caching_list(r->name, r->data_type, r->data, c, false);
+}
+
 ////////////////////////////////////////////////////////////////
 //
 //  HttpConfig::startup()
@@ -1146,6 +1196,8 @@ HttpConfig::startup()
   HttpEstablishStaticConfigLongLong(c.oride.negative_caching_lifetime, "proxy.config.http.negative_caching_lifetime");
   HttpEstablishStaticConfigByte(c.oride.negative_revalidating_enabled, "proxy.config.http.negative_revalidating_enabled");
   HttpEstablishStaticConfigLongLong(c.oride.negative_revalidating_lifetime, "proxy.config.http.negative_revalidating_lifetime");
+  RecRegisterConfigUpdateCb("proxy.config.http.negative_caching_list", &negative_caching_list_cb, &c);
+  RecLookupRecord("proxy.config.http.negative_caching_list", &load_negative_caching_var, &c, true);
 
   // Buffer size and watermark
   HttpEstablishStaticConfigLongLong(c.oride.default_buffer_size_index, "proxy.config.http.default_buffer_size");
@@ -1434,6 +1486,8 @@ HttpConfig::reconfigure()
   params->oride.client_cert_filename        = ats_strdup(m_master.oride.client_cert_filename);
   params->oride.client_cert_filepath        = ats_strdup(m_master.oride.client_cert_filepath);
 
+  params->negative_caching_list = m_master.negative_caching_list;
+
   m_id = configProcessor.set(m_id, params);
 
 #undef INT_TO_BOOL
diff --git a/proxy/http/HttpConfig.h b/proxy/http/HttpConfig.h
index 3518dce..3057f58 100644
--- a/proxy/http/HttpConfig.h
+++ b/proxy/http/HttpConfig.h
@@ -50,6 +50,9 @@
 #include "ProxyConfig.h"
 #include "P_RecProcess.h"
 
+static const unsigned HTTP_STATUS_NUMBER = 600;
+using HttpStatusBitset                   = std::bitset<HTTP_STATUS_NUMBER>;
+
 /* Instead of enumerating the stats in DynamicStats.h, each module needs
    to enumerate its stats separately and register them with librecords
    */
@@ -857,6 +860,9 @@ public:
 
   MgmtByte server_session_sharing_pool = TS_SERVER_SESSION_SHARING_POOL_THREAD;
 
+  // bitset to hold the status codes that will BE cached with negative caching enabled
+  HttpStatusBitset negative_caching_list;
+
   // All the overridable configurations goes into this class member, but they
   // are not copied over until needed ("lazy").
   OverridableHttpConfigParams oride;
diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc
index 83a98b1..c8bb547 100644
--- a/proxy/http/HttpTransact.cc
+++ b/proxy/http/HttpTransact.cc
@@ -239,24 +239,15 @@ is_negative_caching_appropriate(HttpTransact::State *s)
     return false;
   }
 
-  switch (s->hdr_info.server_response.status_get()) {
-  case HTTP_STATUS_NO_CONTENT:
-  case HTTP_STATUS_USE_PROXY:
-  case HTTP_STATUS_FORBIDDEN:
-  case HTTP_STATUS_NOT_FOUND:
-  case HTTP_STATUS_METHOD_NOT_ALLOWED:
-  case HTTP_STATUS_REQUEST_URI_TOO_LONG:
-  case HTTP_STATUS_INTERNAL_SERVER_ERROR:
-  case HTTP_STATUS_NOT_IMPLEMENTED:
-  case HTTP_STATUS_BAD_GATEWAY:
-  case HTTP_STATUS_SERVICE_UNAVAILABLE:
-  case HTTP_STATUS_GATEWAY_TIMEOUT:
+  int status  = s->hdr_info.server_response.status_get();
+  auto params = s->http_config_param;
+  if (params->negative_caching_list[status]) {
+    TxnDebug("http_trans", "%d is eligible for negative caching", status);
     return true;
-  default:
-    break;
+  } else {
+    TxnDebug("http_trans", "%d is NOT eligible for negative caching", status);
+    return false;
   }
-
-  return false;
 }
 
 inline static HttpTransact::LookingUp_t