[
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)
