[GitHub] [trafficserver] bneradt commented on a diff in pull request #9018: dns error logging to diags

[GitBox <gi...@apache.org>]

bneradt commented on code in PR #9018:
URL: https://github.com/apache/trafficserver/pull/9018#discussion_r961051284


##########
iocore/dns/DNS.cc:
##########
@@ -32,6 +32,8 @@
 #define SRV_SERVER (RRFIXEDSZ + 6)
 #define SRV_FIXEDSZ (RRFIXEDSZ + 6)
 
+#define RCODE_ARRAY_SIZE 11
+

Review Comment:
   We don't need `RCODE_ARRAY_SIZE`. If we initialize an array without an explicit size, the compiler figures out the size for us. Thus:
   
   ```
     const char *RCODE_NAME[] = {
        "NOERROR", "FORMERR", "SERVFAIL", "NXDOMAIN", "NOTIMP", "REFUSED", "YXDOMAIN", "YXRRSET", "NXRRSET", "NOTAUTH", "NOTZONE",
      };
   ```



##########
iocore/dns/DNS.cc:
##########
@@ -1553,31 +1573,36 @@ dns_process(DNSHandler *handler, HostEnt *buf, int len)
     goto Lerror;
   }
 
+  // Logs using SiteThrottled* version is helpful for noisy logs, being used here
+  // instead of print statements to help with the possible retries when a dns code occurs
   if (h->rcode != NOERROR || !h->ancount) {
     Debug("dns", "received rcode = %d", h->rcode);
     switch (h->rcode) {
     default:
-      Warning("Unknown DNS error %d for [%s]", h->rcode, e->qname);
+      SiteThrottledWarning("UNKNOWN: DNS error %d for [%s]", h->rcode, e->qname);
       retry     = true;
       server_ok = false; // could be server problems
       goto Lerror;
+    case NOERROR: // Included for completeness. The above condition ensures that NOERROR should not enter this block.
+      Debug("dns", "%s: DNS error %d for [%s]: %s", RCODE_NAME[h->rcode], h->rcode, e->qname, RCODE_DESCRIPTION[h->rcode]);
+      break; // need to break here or this fails test: proxy_serve_stale_dns_fail even though NOERROR is not used in this block.

Review Comment:
   I see. The NOERROR can enter here if `!h->ancount` is true. So this comment, which I think I suggested to you, actually doesn't make sense.
   
   I suggest removing the two comments you've added on these lines but leave the case handling NOERROR. I assume changing `NOERROR` to a Debug is a good change since we don't need to warn about it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org