Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Run RemoteReporter's flush timer in daemon thread. #124

Merged
merged 2 commits into from
Feb 1, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2016, Uber Technologies, Inc
* Copyright (c) 2017, Uber Technologies, Inc
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -31,6 +31,7 @@
import java.util.TimerTask;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.atomic.AtomicLong;
import lombok.ToString;
import lombok.extern.slf4j.Slf4j;

Expand All @@ -47,6 +48,7 @@ public class RemoteReporter implements Reporter {
private final Sender sender;
private final int maxQueueSize;
private final Metrics metrics;
private static final AtomicLong nextSerialNumber = new AtomicLong(0);

/*
* RemoteReporter takes a Sender object, and sends spans for a specific protocol, and transport.
Expand All @@ -59,12 +61,14 @@ public RemoteReporter(final Sender sender, int flushInterval, int maxQueueSize,
commandQueue = new ArrayBlockingQueue<>(maxQueueSize);

// start a thread to append spans
long serialNumber = nextSerialNumber.getAndIncrement();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why we need this. In a real application there would be only one thread with each name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seemed like a good idea to ensure thread name uniqueness, but you're right, it's not necessary. This is my first pull request on GitHub: do I make the change in my repo then push, or do you do it here directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you make the change in the same branch as you did and just push that branch, the PR should be automatically updated.

queueProcessor = new QueueProcessor();
queueProcessorThread = new Thread(queueProcessor);
queueProcessorThread =
new Thread(queueProcessor, "jaeger.RemoteReporter-QueueProcessor-" + serialNumber);
queueProcessorThread.setDaemon(true);
queueProcessorThread.start();

flushTimer = new Timer();
flushTimer = new Timer("jaeger.RemoteReporter-FlushTimer-" + serialNumber, true /* isDaemon */);
flushTimer.schedule(
new TimerTask() {
@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2016, Uber Technologies, Inc
* Copyright (c) 2017, Uber Technologies, Inc
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -34,6 +34,8 @@
import java.util.List;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class RemoteReporterTest {
private Reporter reporter;
Expand Down Expand Up @@ -89,4 +91,17 @@ public void testRemoteReporterFlushesOnClose() throws Exception {
assertEquals(
100L, metricsReporter.counters.get("jaeger.traces.sampled=y.state=started").longValue());
}

@Test
public void testRemoteReporterFlushTimerThread() throws Exception {
int flushTimerThreadCount = 0;
for (Thread thread : Thread.getAllStackTraces().keySet()) {
if (!thread.getName().startsWith("jaeger.RemoteReporter-FlushTimer")) {
continue;
}
++flushTimerThreadCount;
assertTrue(thread.isDaemon());
}
assertFalse(flushTimerThreadCount == 0);
}
}