Ethanlm commented on a change in pull request #3289:
URL: https://github.com/apache/storm/pull/3289#discussion_r445723573
##########
File path:
examples/storm-perf/src/main/java/org/apache/storm/perf/queuetest/JCQueuePerfTest.java
##########
@@ -72,7 +72,7 @@ private static void producerFwdConsumer(int prodBatchSz) {
private static void oneProducer1Consumer(int prodBatchSz) {
- JCQueue q1 = new JCQueue("q1", 50_000, 0, prodBatchSz, new
WaitStrategyPark(100), "test", "test",
+ JCQueue q1 = new JCQueue("q1", null, 50_000, 0, prodBatchSz, new
WaitStrategyPark(100), "test", "test",
Review comment:
why is this `null` (Although I guess it doesn't matter)
##########
File path:
storm-client/test/jvm/org/apache/storm/utils/JCQueueBackpressureTest.java
##########
@@ -22,7 +22,7 @@
public class JCQueueBackpressureTest {
private static JCQueue createQueue(String name, int queueSize) {
- return new JCQueue(name, queueSize, 0, 1, new WaitStrategyPark(0),
"test", "test", Collections.singletonList(1000), 1000, new
StormMetricRegistry());
+ return new JCQueue(name, null, queueSize, 0, 1, new
WaitStrategyPark(0), "test", "test", Collections.singletonList(1000), 1000, new
StormMetricRegistry());
Review comment:
is it better to use `name` instead of `null`?
##########
File path: storm-client/src/jvm/org/apache/storm/utils/JCQueue.java
##########
@@ -44,15 +44,16 @@
private final IWaitStrategy backPressureWaitStrategy;
private final String queueName;
- public JCQueue(String queueName, int size, int overflowLimit, int
producerBatchSz, IWaitStrategy backPressureWaitStrategy,
- String topologyId, String componentId, List<Integer>
taskIds, int port, StormMetricRegistry metricRegistry) {
+ public JCQueue(String queueName, String metricNamePrefix, int size, int
overflowLimit, int producerBatchSz,
+ IWaitStrategy backPressureWaitStrategy, String topologyId,
String componentId, List<Integer> taskIds,
+ int port, StormMetricRegistry metricRegistry) {
this.queueName = queueName;
this.overflowLimit = overflowLimit;
this.recvQueue = new MpscArrayQueue<>(size);
this.overflowQ = new MpscUnboundedArrayQueue<>(size);
for (Integer taskId : taskIds) {
- this.jcqMetrics.add(new JCQueueMetrics(queueName, topologyId,
componentId, taskId, port,
+ this.jcqMetrics.add(new JCQueueMetrics(metricNamePrefix,
topologyId, componentId, taskId, port,
Review comment:
Since JCQueueMetrics takes `metricNamePrefix` now, I think it is better
to change the variable name in JCQueueMetrics from `queueName` to
`metricNamePrefix` to avoid confusion.
https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/utils/JCQueueMetrics.java#L35
##########
File path: storm-client/test/jvm/org/apache/storm/utils/JCQueueTest.java
##########
@@ -157,7 +157,7 @@ private JCQueue createQueue(String name, int queueSize) {
}
private JCQueue createQueue(String name, int batchSize, int queueSize) {
- return new JCQueue(name, queueSize, 0, batchSize, waitStrategy,
"test", "test", Collections.singletonList(1000), 1000, new
StormMetricRegistry());
+ return new JCQueue(name, null, queueSize, 0, batchSize, waitStrategy,
"test", "test", Collections.singletonList(1000), 1000, new
StormMetricRegistry());
Review comment:
is it better to use name instead of null?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
