Review comments.

Review comments.

Author: sureshbabu-areekara

presto-client/src/main/java/com/facebook/presto/client/okhttp3/internal/tls/DistinguishedNameParser.java

@@ -30,6 +30,9 @@
 import java.util.Locale;
 import java.util.regex.Pattern;
 
+/**
+ * The type Legacy hostname verifier.
+ */
 public class LegacyHostnameVerifier
         implements HostnameVerifier
 {
@@ -38,6 +41,9 @@ public class LegacyHostnameVerifier
     private static final Pattern VERIFY_AS_IP_ADDRESS = Pattern.compile(
             "([0-9a-fA-F]*:[0-9a-fA-F:.]*)|([\\d.]+)");
 
+    /**
+     * The constant INSTANCE.
+     */
     public static final HostnameVerifier INSTANCE = new LegacyHostnameVerifier();
 
     private LegacyHostnameVerifier()
@@ -51,23 +57,19 @@ public boolean verify(String host, SSLSession session)
             return true;
         }
 
-        // the CN cannot be used with IP addresses
         if (verifyAsIpAddress(host)) {
             return false;
         }
 
-        // try to verify using the legacy CN rules
         try {
             Certificate[] certificates = session.getPeerCertificates();
             X509Certificate certificate = (X509Certificate) certificates[0];
 
-            // only use CN if there are no alt names
             if (!allSubjectAltNames(certificate).isEmpty()) {
                 return false;
             }
 
             X500Principal principal = certificate.getSubjectX500Principal();
-            // RFC 2818 advises using the most specific name for matching.
             String cn = new DistinguishedNameParser(principal).findMostSpecific("cn");
             if (cn != null) {
                 return verifyHostName(host, cn);
@@ -80,49 +82,23 @@ public boolean verify(String host, SSLSession session)
         }
     }
 
+    /**
+     * Verify as ip address boolean.
+     *
+     * @param host the host
+     * @return the boolean
+     */
     static boolean verifyAsIpAddress(String host)
     {
         return VERIFY_AS_IP_ADDRESS.matcher(host).matches();
     }
 
     /**
-     * Returns true if {@code certificate} matches {@code ipAddress}.
+     * All subject alt names list.
+     *
+     * @param certificate the certificate
+     * @return the list
      */
-    private boolean verifyIpAddress(String ipAddress, X509Certificate certificate)
-    {
-        List<String> altNames = getSubjectAltNames(certificate, ALT_IPA_NAME);
-        for (int i = 0, size = altNames.size(); i < size; i++) {
-            if (ipAddress.equalsIgnoreCase(altNames.get(i))) {
-                return true;
-            }
-        }
-        return false;
-    }
-
-    private boolean verifyHostName(String hostName, X509Certificate certificate)
-    {
-        hostName = hostName.toLowerCase(Locale.US);
-        boolean hasDns = false;
-        List<String> altNames = getSubjectAltNames(certificate, ALT_DNS_NAME);
-        for (int i = 0, size = altNames.size(); i < size; i++) {
-            hasDns = true;
-            if (verifyHostName(hostName, altNames.get(i))) {
-                return true;
-            }
-        }
-
-        if (!hasDns) {
-            X500Principal principal = certificate.getSubjectX500Principal();
-            // RFC 2818 advises using the most specific name for matching.
-            String cn = new DistinguishedNameParser(principal).findMostSpecific("cn");
-            if (cn != null) {
-                return verifyHostName(hostName, cn);
-            }
-        }
-
-        return false;
-    }
-
     public static List<String> allSubjectAltNames(X509Certificate certificate)
     {
         List<String> altIpaNames = getSubjectAltNames(certificate, ALT_IPA_NAME);
@@ -173,90 +149,50 @@ private static List<String> getSubjectAltNames(X509Certificate certificate, int
      */
     private boolean verifyHostName(String hostName, String pattern)
     {
-        // Basic sanity checks
-        // Check length == 0 instead of .isEmpty() to support Java 5.
         if ((hostName == null) || (hostName.length() == 0) || (hostName.startsWith("."))
                 || (hostName.endsWith(".."))) {
-            // Invalid domain name
             return false;
         }
         if ((pattern == null) || (pattern.length() == 0) || (pattern.startsWith("."))
                 || (pattern.endsWith(".."))) {
-            // Invalid pattern/domain name
             return false;
         }
 
-        // Normalize hostName and pattern by turning them into absolute domain names if they are not
-        // yet absolute. This is needed because server certificates do not normally contain absolute
-        // names or patterns, but they should be treated as absolute. At the same time, any hostName
-        // presented to this method should also be treated as absolute for the purposes of matching
-        // to the server certificate.
-        //   www.android.com  matches www.android.com
-        //   www.android.com  matches www.android.com.
-        //   www.android.com. matches www.android.com.
-        //   www.android.com. matches www.android.com
         if (!hostName.endsWith(".")) {
             hostName += '.';
         }
         if (!pattern.endsWith(".")) {
             pattern += '.';
         }
-        // hostName and pattern are now absolute domain names.
 
         pattern = pattern.toLowerCase(Locale.US);
-        // hostName and pattern are now in lower case -- domain names are case-insensitive.
 
         if (!pattern.contains("*")) {
-            // Not a wildcard pattern -- hostName and pattern must match exactly.
             return hostName.equals(pattern);
         }
-        // Wildcard pattern
-
-        // WILDCARD PATTERN RULES:
-        // 1. Asterisk (*) is only permitted in the left-most domain name label and must be the
-        //    only character in that label (i.e., must match the whole left-most label).
-        //    For example, *.example.com is permitted, while *a.example.com, a*.example.com,
-        //    a*b.example.com, a.*.example.com are not permitted.
-        // 2. Asterisk (*) cannot match across domain name labels.
-        //    For example, *.example.com matches test.example.com but does not match
-        //    sub.test.example.com.
-        // 3. Wildcard patterns for single-label domain names are not permitted.
 
         if ((!pattern.startsWith("*.")) || (pattern.indexOf('*', 1) != -1)) {
-            // Asterisk (*) is only permitted in the left-most domain name label and must be the only
-            // character in that label
             return false;
         }
 
-        // Optimization: check whether hostName is too short to match the pattern. hostName must be at
-        // least as long as the pattern because asterisk must match the whole left-most label and
-        // hostName starts with a non-empty label. Thus, asterisk has to match one or more characters.
         if (hostName.length() < pattern.length()) {
-            // hostName too short to match the pattern.
             return false;
         }
 
         if ("*.".equals(pattern)) {
-            // Wildcard pattern for single-label domain name -- not permitted.
             return false;
         }
 
-        // hostName must end with the region of pattern following the asterisk.
         String suffix = pattern.substring(1);
         if (!hostName.endsWith(suffix)) {
-            // hostName does not end with the suffix
             return false;
         }
 
-        // Check that asterisk did not match across domain name labels.
         int suffixStartIndexInHostName = hostName.length() - suffix.length();
         if ((suffixStartIndexInHostName > 0)
                 && (hostName.lastIndexOf('.', suffixStartIndexInHostName - 1) != -1)) {
-            // Asterisk is matching across domain name labels -- not permitted.
             return false;
         }
-
-        // hostName matches pattern
         return true;
     }
 }

presto-client/src/main/java/com/facebook/presto/client/okhttp3/internal/tls/LegacyHostnameVerifier.java

@@ -19,7 +19,7 @@
 import static java.util.Objects.requireNonNull;
 
 /**
- * The type TelemetryConfig to store all the values in telemetry.properties.
+ * The type TelemetryConfig to store the values from telemetry-tracing.properties.
  */
 public class TelemetryConfig
 {
@@ -31,11 +31,11 @@ public class TelemetryConfig
     private Integer exporterTimeout;
     private Integer scheduleDelay;
     private Double samplingRatio;
-    private Boolean tracingEnabled = false;
-    private Boolean spanSampling = false;
+    private boolean tracingEnabled;
+    private boolean spanSampling;
 
     /**
-     * The type Telemetry config constants.
+     * The type TelemetryConfigConstants to store constants.
      */
     public static class TelemetryConfigConstants
     {
@@ -54,7 +54,7 @@ private TelemetryConfig()
     }
 
     /**
-     * Gets telemetry config.
+     * Gets the singleton telemetryConfig.
      *
      * @return the telemetry config
      */
@@ -65,7 +65,7 @@ public static TelemetryConfig getTelemetryConfig()
     }
 
     /**
-     * Sets telemetry properties.
+     * Sets telemetry properties from the input.
      *
      * @param telemetryProperties the telemetry properties
      */
@@ -91,91 +91,46 @@ public void setTracingEnabled(Boolean tracingEnabled)
         getTelemetryConfig().tracingEnabled = tracingEnabled;
     }
 
-    /**
-     * Sets span sampling.
-     *
-     * @param spanSampling the span sampling
-     */
     public void setSpanSampling(Boolean spanSampling)
     {
         getTelemetryConfig().spanSampling = spanSampling;
     }
 
-    /**
-     * Gets exporter endpoint.
-     *
-     * @return the exporter endpoint
-     */
     public String getTracingBackendUrl()
     {
         return this.tracingBackendUrl;
     }
 
-    /**
-     * Gets max exporter batch size.
-     *
-     * @return the max exporter batch size
-     */
     public Integer getMaxExporterBatchSize()
     {
         return this.maxExporterBatchSize;
     }
 
-    /**
-     * Gets max queue size.
-     *
-     * @return the max queue size
-     */
     public Integer getMaxQueueSize()
     {
         return this.maxQueueSize;
     }
 
-    /**
-     * Gets exporter timeout.
-     *
-     * @return the exporter timeout
-     */
     public Integer getExporterTimeout()
     {
         return this.exporterTimeout;
     }
 
-    /**
-     * Gets schedule delay.
-     *
-     * @return the schedule delay
-     */
     public Integer getScheduleDelay()
     {
         return this.scheduleDelay;
     }
 
-    /**
-     * Gets sampling ratio.
-     *
-     * @return the sampling ratio
-     */
     public Double getSamplingRatio()
     {
         return this.samplingRatio;
     }
 
-    /**
-     * Gets tracing enabled.
-     *
-     * @return the tracing enabled
-     */
     public static Boolean getTracingEnabled()
     {
         return getTelemetryConfig().tracingEnabled;
     }
 
-    /**
-     * Gets span sampling.
-     *
-     * @return the span sampling
-     */
     public static Boolean getSpanSampling()
     {
         return getTelemetryConfig().spanSampling;

presto-common/src/main/java/com/facebook/presto/common/TelemetryConfig.java

@@ -19,7 +19,7 @@
 import javax.annotation.concurrent.Immutable;
 
 /**
- * The type Tracing config.
+ * POJO to use with TelemetryResource for the dynamically enable/disable the trace endpoint.
  */
 @Immutable
 public class TracingConfig
@@ -38,11 +38,6 @@ public TracingConfig(
         this.tracingEnabled = tracingEnabled;
     }
 
-    /**
-     * Is tracing enabled boolean.
-     *
-     * @return the boolean
-     */
     @JsonProperty
     public boolean isTracingEnabled()
     {

presto-common/src/main/java/com/facebook/presto/common/TracingConfig.java

@@ -39,7 +39,6 @@
 import com.facebook.presto.sql.analyzer.CTEInformationCollector;
 import com.facebook.presto.sql.planner.optimizations.OptimizerInformationCollector;
 import com.facebook.presto.sql.planner.optimizations.OptimizerResultCollector;
-import com.facebook.presto.telemetry.TracingManager;
 import com.facebook.presto.transaction.TransactionManager;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -67,6 +66,8 @@
 import static com.facebook.presto.spi.ConnectorId.createInformationSchemaConnectorId;
 import static com.facebook.presto.spi.ConnectorId.createSystemTablesConnectorId;
 import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
+import static com.facebook.presto.telemetry.TracingManager.getInvalidSpan;
+import static com.facebook.presto.telemetry.TracingManager.spanString;
 import static com.facebook.presto.util.Failures.checkCondition;
 import static com.google.common.base.MoreObjects.toStringHelper;
 import static com.google.common.base.Preconditions.checkArgument;
@@ -530,7 +531,7 @@ public String toString()
     {
         return toStringHelper(this)
                 .add("queryId", queryId)
-                .add("querySpan", TracingManager.spanString(querySpan).orElse(null))
+                .add("querySpan", spanString(querySpan).orElse(null))
                 .add("rootSpan", rootSpan.toString())
                 .add("transactionId", transactionId)
                 .add("user", getUser())
@@ -563,8 +564,8 @@ public static SessionBuilder builder(Session session)
     public static class SessionBuilder
     {
         private QueryId queryId;
-        private BaseSpan querySpan = TracingManager.getInvalidSpan(); //do not initialize with null
-        private BaseSpan rootSpan = TracingManager.getInvalidSpan();  //do not initialize with null
+        private BaseSpan querySpan = getInvalidSpan(); //do not initialize with null
+        private BaseSpan rootSpan = getInvalidSpan();  //do not initialize with null
         private TransactionId transactionId;
         private boolean clientTransactionSupport;
         private Identity identity;

presto-main/src/main/java/com/facebook/presto/Session.java

@@ -19,9 +19,6 @@
 
 import static org.weakref.jmx.guice.ExportBinder.newExporter;
 
-/**
- * The type Telemetry module.
- */
 public class TelemetryModule
         implements Module
 {