[GitHub] [phoenix] dbwong commented on a change in pull request #1282: PHOENIX-5215 opentelemetry changes replacing htrace - WIP

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

dbwong commented on a change in pull request #1282:
URL: https://github.com/apache/phoenix/pull/1282#discussion_r730601037



##########
File path: phoenix-core/src/it/java/org/apache/shell/TestFSShellCluster.java
##########
@@ -0,0 +1,40 @@
+package org.apache.shell;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FsShell;
+import org.apache.hadoop.fs.LocatedFileStatus;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.RemoteIterator;
+import org.apache.hadoop.fs.permission.FsPermission;
+import org.apache.hadoop.util.Progressable;
+
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.net.URI;
+import java.nio.file.Paths;
+import java.util.List;
+
+public class TestFSShellCluster {

Review comment:
       Same with this class on testing and IT

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java
##########
@@ -279,9 +281,7 @@ public void close() throws IOException {
                             try {
                                 delegate.close();
                             } finally {
-                                if (child != null) {
-                                    child.stop();
-                                }
+                                child.end();

Review comment:
       What did we change here that insure child is non-null? 

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/iterate/ParallelIterators.java
##########
@@ -108,6 +108,7 @@ protected void submitWork(final List<List<Scan>> nestedScans, List<List<Pair<Sca
         context.getOverallQueryMetrics().updateNumParallelScans(numScans);
         GLOBAL_NUM_PARALLEL_SCANS.update(numScans);
         final long renewLeaseThreshold = context.getConnection().getQueryServices().getRenewLeaseThresholdMilliSeconds();
+        //TODO: Parent Span creation using Span.current() and create children with description  "Parallel scanner for table: " + tableRef.getTable().getPhysicalName().getString()

Review comment:
       Is this being handled in this JIRA or a followup?

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/iterate/ParallelIterators.java
##########
@@ -118,10 +119,11 @@ protected void submitWork(final List<List<Scan>> nestedScans, List<List<Pair<Sca
                         mutationState, tableRef, scan, scanMetricsHolder, renewLeaseThreshold, plan,
                         scanGrouper, caches);
             context.getConnection().addIteratorForLeaseRenewal(tableResultItr);
-            Future<PeekingResultIterator> future = executor.submit(Tracing.wrap(new JobCallable<PeekingResultIterator>() {
+            Future<PeekingResultIterator> future = executor.submit(new JobCallable<PeekingResultIterator>() {
                 
                 @Override
                 public PeekingResultIterator call() throws Exception {
+                    //TODO: create child span for each call using parent

Review comment:
       This one we use to handle with Tracing.wrap right?

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/trace/TraceReader.java
##########
@@ -47,6 +46,7 @@
 public class TraceReader {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(TraceReader.class);
+    public static final long ROOT_SPAN_ID = 0x74ace;

Review comment:
       Is this the recommended approach for a top level span?

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/LocalConnectionTest.java
##########
@@ -0,0 +1,80 @@
+package org.apache.phoenix.end2end;
+
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.context.Scope;
+import org.apache.phoenix.trace.TraceUtil;
+
+import java.sql.*;
+
+public class LocalConnectionTest {

Review comment:
       Is this class assuming a local instance is running?  Can this be made an actual IT?

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/LocalConnectionTest.java
##########
@@ -0,0 +1,80 @@
+package org.apache.phoenix.end2end;
+
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.context.Scope;
+import org.apache.phoenix.trace.TraceUtil;
+
+import java.sql.*;

Review comment:
       nit: Phoenix style guidelines does not want to use import *




-- 
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: issues-unsubscribe@phoenix.apache.org

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