phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Karan Mehta (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (PHOENIX-3752) Remove hadoop metrics integration from the tracing framework
Date Thu, 06 Apr 2017 23:21:41 GMT

    [ https://issues.apache.org/jira/browse/PHOENIX-3752?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15959955#comment-15959955
] 

Karan Mehta commented on PHOENIX-3752:
--------------------------------------

Thanks [~samarthjain] for reviewing the patch. Some answers to the questions you have pointed
out. 

bq. I think a better name TraceWriterExtends would be something like TestTraceWriter to tell
the reader that this is a test-only extension.
Will change the name. Also any other suggestions for {{TraceMetricSource}}, in an earlier
JIRA we were planning to change that one as well. Is {{TraceSource}} acceptable?

bq. This looks like an abstraction leak (in PhoenixTraceReaderIT). Classes outside TraceWriter
don't need to know the state of the span queue.
{code}
if(traceWriterExtends.getSpanQueue() == null)
+            traceWriterExtends.initSpanQueue(10);
{code}

Since there is no instance of {{PhoenixConnection}} class in this test, the JVM never loads
the class, thus its static method is never called. This {{TraceMetricSource}} is not initialized
and so does {{spanQueue}} as well. That is why I put in two new functions to initialize the
size and are also annotated as {{@VisibleForTesting}} purposes since technically it should
never leak out the abstraction as you suggested.
This test also brings in the question that, do we really want to have the static initialization
inside the {{PhoenixConnection}} class or not? Although we can assume that user has created
at least one instance of {{PhoenixConnection}} before accessing any Phoenix capabilities.

If required, we can also remove this test, since it is pretty naive in itself though.

bq. I think these would be better defaults:
{code}
+    public static final int DEFAULT_TRACING_BATCH_SIZE = 100;
+    public static final int DEFAULT_TRACING_TRACE_BUFFER_SIZE = 1000;
{code}

Will update these values.

bq. Please fix the formatting of the Apache header in TraceMetricSource.java
Will do.

bq. Make CAPACITY final in TraceMetricSource
Will do.

bq. This will cause excessive logging
{code}
+    @Override
+    public void receiveSpan(Span span) {
+        if (spanQueue.offer(span))
+            LOG.info("Span buffered to queue " + span.toJson());
+        else
+            LOG.info("Span NOT buffered due to overflow in queue " + span.toJson());
{code}
Will make required changes as suggested. 

bq. Are we expecting the user of TraceWriter to explicitly call initSpanQueue? That doesn't
seem right to me. Couldn't the init be taken care of by the TraceWriter itself? For tests,
won't the default queue capacity be sufficient?
{code}
+    @VisibleForTesting
+    public void initSpanQueue(int capacity) {
+        if(spanQueue != null) {
+            spanQueue = new ArrayBlockingQueue<>(capacity);
+        }
+    }
{code}

This is again related to the particular test discussed above. 

bq. Who is calling TraceWriter#stop()? Also we shouldn't be swallowing InterruptedExceptions
This function call is only used by tests. When multiple tests run, if the thread pools from
previous tests are lying around, they pickup the items from the spanQueue and commit it to
the table on a different connection. This results in {{CountDownLatch}} of the current test
not being decremented, since that {{Connection}} is not being used to commit. Thus the tests
will pass if run individually, but may have a chance of failing if ran in a group.

Please also clarify as to what you meant by swallowing exceptions. How do we typically handle
exceptions in Phoenix? Same goes for this one as well.

bq. We shouldn't be swallowing exceptions by printing the stacktrace. Either let the method
throw the exception for the callers to handle the exception or log an error with this exception
if we don't want the future task executions to be suppressed because of unhandled exceptions.
{code}
+    protected void commitBatch(Connection conn) {
+        try {
+            conn.commit();
+        } catch (SQLException e) {
+            e.printStackTrace();
+        }
+    }
{code}

bq. Please remove System.out debug statements
{code}
public FlushMetrics() {
+            conn = getConnection(tableName);
+            System.out
+                    .println("New Thread Created: " + conn.hashCode() + " tableName: " +
tableName);
+        }
{code}

Missed out on it. Will remove it.

> Remove hadoop metrics integration from the tracing framework
> ------------------------------------------------------------
>
>                 Key: PHOENIX-3752
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-3752
>             Project: Phoenix
>          Issue Type: Improvement
>            Reporter: Samarth Jain
>            Assignee: Karan Mehta
>         Attachments: PHOENIX-3752.patch
>
>
> See discussion on PHOENIX-3062. We don't really need to use the hadoop metrics framework
for writing traces to our trace table.
> [~karanmehta93] - let's use this JIRA instead of PHOENIX-3062. We can close that one
once your this work is in.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Mime
View raw message