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

    https://github.com/apache/storm/pull/2538#discussion_r165857620
  
    --- Diff: 
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/internal/CommitMetadataManager.java
 ---
    @@ -0,0 +1,90 @@
    +/*
    + * Copyright 2018 The Apache Software Foundation.
    + *
    + * Licensed under the Apache License, Version 2.0 (the "License");
    + * you may not use this file except in compliance with the License.
    + * You may obtain a copy of the License at
    + *
    + *      http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.storm.kafka.spout.internal;
    +
    +import com.fasterxml.jackson.core.JsonProcessingException;
    +import com.fasterxml.jackson.databind.ObjectMapper;
    +import java.io.IOException;
    +import java.util.Map;
    +import org.apache.kafka.clients.consumer.OffsetAndMetadata;
    +import org.apache.kafka.common.TopicPartition;
    +import org.apache.storm.kafka.spout.KafkaSpout;
    +import org.apache.storm.kafka.spout.KafkaSpoutConfig.ProcessingGuarantee;
    +import org.apache.storm.task.TopologyContext;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +/**
    + * Generates and reads commit metadata.
    + */
    +public class CommitMetadataManager {
    +
    +    private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
    +    private static final Logger LOG = 
LoggerFactory.getLogger(CommitMetadata.class);
    +    // Metadata information to commit to Kafka. It is unique per spout 
instance.
    +    private final String commitMetadata;
    +    private final ProcessingGuarantee processingGuarantee;
    +    private final TopologyContext context;
    +
    +    /**
    +     * Create a manager with the given context.
    +     */
    +    public CommitMetadataManager(TopologyContext context, 
ProcessingGuarantee processingGuarantee) {
    +        this.context = context;
    +        try {
    +            commitMetadata = JSON_MAPPER.writeValueAsString(new 
CommitMetadata(
    --- End diff --
    
    It's bad practice to create objects in the constructor. I simply don't do 
it. However, it's done in a lot of places in Storm already, so I am OK if you 
want to leave it as is. 
    
    Another example why this could potentially be bad is if someone wants do 
subclass this class. If we leave it like this, perhaps the class should be 
final then.
    
    These are just some suggestions. You can  leave it as is or go with either 
of of the suggestions.


---

Reply via email to