You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Guozhang Wang <gu...@linkedin.com> on 2014/04/04 23:33:50 UTC
Review Request 20050: Fix KAFKA-1359
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1359
https://issues.apache.org/jira/browse/KAFKA-1359
Repository: kafka
Description
-------
KAFKA-1359.v1
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
Diff: https://reviews.apache.org/r/20050/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20050: Fix KAFKA-1359: move producer metrics into a
centalized class; fix some bugs in SenderTest
Posted by Guozhang Wang <gu...@linkedin.com>.
> On April 10, 2014, 11:58 p.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java, lines 799-803
> > <https://reviews.apache.org/r/20050/diff/2/?file=554492#file554492line799>
> >
> > Could we register this in ProducerMetrics too? We could pass Sender in when constructing ProducerMetrics.
Jun, we cannot really do that since ProducerMetrics is in comm while Sender is in clients.
The reason I put ProducerMetrics in comm is that it needs to be accessed by Selector, which is in comm.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/#review40106
-----------------------------------------------------------
On April 10, 2014, 5:11 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20050/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 5:11 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1359
> https://issues.apache.org/jira/browse/KAFKA-1359
>
>
> Repository: kafka
>
>
> Description
> -------
>
> remove un-used imports
>
>
> move registration to centralized ProducerMetrics; fixed a small bug in SenderTest
>
>
> KAFKA-1359.v2
>
>
> KAFKA-1359.v1
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java ffd13ff00ba0d0d969e2ed130e49053f02481b50
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
> clients/src/main/java/org/apache/kafka/common/ProducerMetrics.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
> clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java a2b77226f8c58caf632a0f4665bd4e4cd93e643d
> clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 5c5e3d40819e41cab7b52a0eeaee5f2e7317b7b3
>
> Diff: https://reviews.apache.org/r/20050/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20050: Fix KAFKA-1359: move producer metrics into a
centalized class; fix some bugs in SenderTest
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/#review40106
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/20050/#comment72967>
Could we register this in ProducerMetrics too? We could pass Sender in when constructing ProducerMetrics.
clients/src/main/java/org/apache/kafka/common/network/Selector.java
<https://reviews.apache.org/r/20050/#comment72963>
Could we register this in ProducerMetrics too? We could pass Selector to ProducerMetrics.
clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java
<https://reviews.apache.org/r/20050/#comment72964>
Should we remove the code?
clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java
<https://reviews.apache.org/r/20050/#comment72965>
Do we need these two run() calls? Doesn't completeSend do that already?
- Jun Rao
On April 10, 2014, 5:11 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20050/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 5:11 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1359
> https://issues.apache.org/jira/browse/KAFKA-1359
>
>
> Repository: kafka
>
>
> Description
> -------
>
> remove un-used imports
>
>
> move registration to centralized ProducerMetrics; fixed a small bug in SenderTest
>
>
> KAFKA-1359.v2
>
>
> KAFKA-1359.v1
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java ffd13ff00ba0d0d969e2ed130e49053f02481b50
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
> clients/src/main/java/org/apache/kafka/common/ProducerMetrics.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
> clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java a2b77226f8c58caf632a0f4665bd4e4cd93e643d
> clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 5c5e3d40819e41cab7b52a0eeaee5f2e7317b7b3
>
> Diff: https://reviews.apache.org/r/20050/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20050: Fix KAFKA-1359: revert to 1st patch, plus some
minor changes; unit test passed
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/
-----------------------------------------------------------
(Updated April 16, 2014, 4:54 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1359: revert to 1st patch, plus some minor changes; unit test passed
Bugs: KAFKA-1359
https://issues.apache.org/jira/browse/KAFKA-1359
Repository: kafka
Description (updated)
-------
Minor hard-written string name fixes
KAFKA-1359.v2
KAFKA-1359.v1
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
Diff: https://reviews.apache.org/r/20050/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20050: Fix KAFKA-1359: incorporate Jun's comments
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/#review40433
-----------------------------------------------------------
The first question is whether we should we centralize all the metrics into a ProducerMetrics class.
Even if we do this, though, this patch seems to try to accomplish this with a class of all static methods/variables. This is definitely, definitely not the right way to go. Among other things I don't see how this would work if you have multiple producers in the same JVM.
With respect to centralizing the metrics, I think this is not what we want to do. The whole point of this metrics design is to allow a cross-cutting concern like metrics to appear all over the code without tying everything together with a single class with all the metrics logic. In other words a lower-level package like Selector can have metrics without implicitly depending on the rest of the producer.
I think there are other ways to solve the registration problem we were seeing...
- Jay Kreps
On April 11, 2014, 9:20 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20050/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 9:20 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1359
> https://issues.apache.org/jira/browse/KAFKA-1359
>
>
> Repository: kafka
>
>
> Description
> -------
>
> minor change in SenderTest
>
>
> remove un-used imports
>
>
> move registration to centralized ProducerMetrics; fixed a small bug in SenderTest
>
>
> KAFKA-1359.v2
>
>
> KAFKA-1359.v1
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java ffd13ff00ba0d0d969e2ed130e49053f02481b50
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
> clients/src/main/java/org/apache/kafka/common/ProducerMetrics.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
> clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java a2b77226f8c58caf632a0f4665bd4e4cd93e643d
> clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 5c5e3d40819e41cab7b52a0eeaee5f2e7317b7b3
>
> Diff: https://reviews.apache.org/r/20050/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20050: Fix KAFKA-1359: incorporate Jun's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/
-----------------------------------------------------------
(Updated April 11, 2014, 9:20 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1359: incorporate Jun's comments
Bugs: KAFKA-1359
https://issues.apache.org/jira/browse/KAFKA-1359
Repository: kafka
Description (updated)
-------
minor change in SenderTest
remove un-used imports
move registration to centralized ProducerMetrics; fixed a small bug in SenderTest
KAFKA-1359.v2
KAFKA-1359.v1
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java ffd13ff00ba0d0d969e2ed130e49053f02481b50
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
clients/src/main/java/org/apache/kafka/common/ProducerMetrics.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java a2b77226f8c58caf632a0f4665bd4e4cd93e643d
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 5c5e3d40819e41cab7b52a0eeaee5f2e7317b7b3
Diff: https://reviews.apache.org/r/20050/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20050: Fix KAFKA-1359: move producer metrics into a
centalized class; fix some bugs in SenderTest
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/
-----------------------------------------------------------
(Updated April 10, 2014, 5:11 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1359: move producer metrics into a centalized class; fix some bugs in SenderTest
Bugs: KAFKA-1359
https://issues.apache.org/jira/browse/KAFKA-1359
Repository: kafka
Description (updated)
-------
remove un-used imports
move registration to centralized ProducerMetrics; fixed a small bug in SenderTest
KAFKA-1359.v2
KAFKA-1359.v1
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java ffd13ff00ba0d0d969e2ed130e49053f02481b50
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
clients/src/main/java/org/apache/kafka/common/ProducerMetrics.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java a2b77226f8c58caf632a0f4665bd4e4cd93e643d
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 5c5e3d40819e41cab7b52a0eeaee5f2e7317b7b3
Diff: https://reviews.apache.org/r/20050/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20050: Fix KAFKA-1359
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/#review39825
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/20050/#comment72489>
Perhaps these can all be wrapped in the same if statement in line 815 to save the unnecessary overhead?
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/20050/#comment72490>
Ditto as the above.
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/20050/#comment72494>
I am wondering if this is the right place to do the registration. Since this is only called when we have producer request, if the first request sent to a node is not a produce request, we won't have pre-created the node level sensor. I think this can happen during a broker failure when the first request to a node could be a metadata request.
So, we probably have to register this in Selector. This may also solve your other problem that a metric is not registered in unit test.
- Jun Rao
On April 4, 2014, 9:33 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20050/
> -----------------------------------------------------------
>
> (Updated April 4, 2014, 9:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1359
> https://issues.apache.org/jira/browse/KAFKA-1359
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1359.v1
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
> clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
>
> Diff: https://reviews.apache.org/r/20050/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20050: Fix KAFKA-1359
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/#review39836
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/20050/#comment72523>
I agree, it is better to register this in Selector. This would not solve the unit tests though since there are Sender unit tests that will be broken then..
- Guozhang Wang
On April 4, 2014, 9:33 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20050/
> -----------------------------------------------------------
>
> (Updated April 4, 2014, 9:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1359
> https://issues.apache.org/jira/browse/KAFKA-1359
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1359.v1
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
> clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
>
> Diff: https://reviews.apache.org/r/20050/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20050: Fix KAFKA-1359
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20050/#review39598
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/20050/#comment72038>
Ideal we should be using Utils.notNull for all these. But we can't since some unit/integration tests bypass the metric registration phase and causing NPE. I tried to create a MockMetrics and MockSensor but that does not work perfectly either. If people have better suggestion I would love to make a change.
- Guozhang Wang
On April 4, 2014, 9:33 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20050/
> -----------------------------------------------------------
>
> (Updated April 4, 2014, 9:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1359
> https://issues.apache.org/jira/browse/KAFKA-1359
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1359.v1
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 855ae84f14aa91653b3fa855c2af40560323f42a
> clients/src/main/java/org/apache/kafka/common/network/Selector.java 558f8b47638b354f9c1a30be5d45dc7b61131bea
>
> Diff: https://reviews.apache.org/r/20050/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>