[ 
https://issues.apache.org/jira/browse/STORM-1015?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14945818#comment-14945818
 ] 

ASF GitHub Bot commented on STORM-1015:
---------------------------------------

Github user choang commented on a diff in the pull request:

    https://github.com/apache/storm/pull/705#discussion_r41323220
  
    --- Diff: external/storm-kafka/src/jvm/storm/kafka/KafkaStateStore.java ---
    @@ -0,0 +1,220 @@
    +package storm.kafka;
    +
    +import com.google.common.collect.Maps;
    +import kafka.api.ConsumerMetadataRequest;
    +import kafka.common.ErrorMapping;
    +import kafka.common.OffsetAndMetadata;
    +import kafka.common.OffsetMetadataAndError;
    +import kafka.common.TopicAndPartition;
    +import kafka.javaapi.*;
    +import kafka.network.BlockingChannel;
    +import org.json.simple.JSONValue;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +import java.util.Map;
    +
    +public class KafkaStateStore implements StateStore {
    +    private static final Logger LOG = 
LoggerFactory.getLogger(KafkaStateStore.class);
    +
    +    private SpoutConfig _spoutConfig;
    +
    +    private int _correlationId = 0;
    +    // https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java
    +    private volatile BlockingChannel _offsetManager;
    +
    +    public KafkaStateStore(Map stormConf, SpoutConfig spoutConfig) {
    --- End diff --
    
    <tt>stormConf</tt> isn't used so remove.  By using <tt>spoutConfig</tt>, 
you are bleeding the spout state into the store.  I understand it is for 
convenience, but using explicit would be better, so perhaps you can create a 
<tt>KafkaStoreConfig</tt>.


> Store Kafka offsets with Kafka's consumer offset management api
> ---------------------------------------------------------------
>
>                 Key: STORM-1015
>                 URL: https://issues.apache.org/jira/browse/STORM-1015
>             Project: Apache Storm
>          Issue Type: Improvement
>          Components: storm-kafka
>    Affects Versions: 0.11.0
>            Reporter: Hang Sun
>            Priority: Minor
>              Labels: consumer, kafka, offset
>   Original Estimate: 72h
>  Remaining Estimate: 72h
>
> Current Kafka spout stores the offsets (and some other states) inside ZK with 
> its proprietary format. This does not work well with other Kafka offset 
> monitoring tools such as Burrow, KafkaOffsetMonitor etc. In addition, the 
> performance does not scale well compared with offsets managed by Kafka's 
> built-in offset management api. I have added a new option for Kafka to store 
> the same data using Kafka's built-in offset management capability. The change 
> is completely backward compatible with the current ZK storage option. The 
> feature can be turned on by a single configuration option. Hope this will 
> help people who wants to explore the option of using Kafka's built-in offset 
> management api.
> References:
> https://cwiki.apache.org/confluence/display/KAFKA/Committing+and+fetching+consumer+offsets+in+Kafka
> https://cwiki.apache.org/confluence/display/KAFKA/A+Guide+To+The+Kafka+Protocol#AGuideToTheKafkaProtocol-OffsetCommit/FetchAPI
> -thanks



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to