[
https://issues.apache.org/jira/browse/PHOENIX-3752?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15959560#comment-15959560
]
Samarth Jain commented on PHOENIX-3752:
---------------------------------------
Thanks for the patch, [~karanmehta93]. Below is some feedback and questions
that I have:
- I think a better name TraceWriterExtends would be something like
TestTraceWriter to tell the reader that this is a test-only extension.
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}
- Why do both the TraceMetricSource and the TraceWriter class expose the
getSpanQueue() method?
- 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}
- Please fix the formatting of the Apache header in TraceMetricSource.java
- Make CAPACITY final in TraceMetricSource
- 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}
I would do something like this:
{code}
@Override
public void receiveSpan(Span span) {
if (spanQueue.offer(span)) {
if (LOG.isTraceEnabled()) {
LOG.trace("Span buffered to queue " + span.toJson());
}
else if (LOG.isDebugEnabled()) {
LOG.debug("Span NOT buffered due to overflow in queue " +
span.toJson());
}
}
{code}
- 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}
- Who is calling TraceWriter#stop()? Also we shouldn't be swallowing
InterruptedExceptions:
{code}
+ public void stop() {
+ try {
+ executor.awaitTermination(5, TimeUnit.SECONDS);
+ executor.shutdownNow();
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ }
{code}
In this case you could possibly just not catch the InterruptedException
(provided the stop method is really needed)
- Please remove System.out debug statements
{code}
public FlushMetrics() {
+ conn = getConnection(tableName);
+ System.out
+ .println("New Thread Created: " + conn.hashCode() + "
tableName: " + tableName);
+ }
{code}
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}
> 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)
