Skip to content

KAFKA-5038: Catch exception#2841

Closed
enothereska wants to merge 4 commits intoapache:0.10.2from
enothereska:KAFKA-5038
Closed

KAFKA-5038: Catch exception#2841
enothereska wants to merge 4 commits intoapache:0.10.2from
enothereska:KAFKA-5038

Conversation

@enothereska
Copy link
Copy Markdown
Contributor

No description provided.

@enothereska
Copy link
Copy Markdown
Contributor Author

@dguy @mjsax for a review. Thanks.

@asfbot
Copy link
Copy Markdown

asfbot commented Apr 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2893/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot
Copy link
Copy Markdown

asfbot commented Apr 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2897/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link
Copy Markdown

asfbot commented Apr 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2892/
Test PASSed (JDK 7 and Scala 2.10).

@enothereska
Copy link
Copy Markdown
Contributor Author

Streams tests also pass on branch builder: https://jenkins.confluent.io/job/system-test-kafka-branch-builder-2/256/

Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

Thanks @eno, LGTM

@dguy
Copy link
Copy Markdown
Contributor

dguy commented Apr 12, 2017

@ijuma can you please merge this?

}
final File lockFile = new File(directoryForTask(taskId), LOCK_FILE_NAME);
try {
lockFile = new File(directoryForTask(taskId), LOCK_FILE_NAME);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it OK that we call directoryForTask with no protection in ProcessorStateManager before we call lock?

        this.baseDir  = stateDirectory.directoryForTask(taskId);
        this.partitionForTopic = new HashMap<>();
        for (TopicPartition source : sources) {
            this.partitionForTopic.put(source.topic(), source);
        }
        this.stores = new LinkedHashMap<>();
        this.globalStores = new HashMap<>();
        this.offsetLimits = new HashMap<>();
        this.restoredOffsets = new HashMap<>();
        this.isStandby = isStandby;
        this.restoreCallbacks = isStandby ? new HashMap<String, StateRestoreCallback>() : null;
        this.storeToChangelogTopic = storeToChangelogTopic;

        this.logPrefix = String.format("task [%s]", taskId);

        if (!stateDirectory.lock(taskId, 5)) {
            throw new LockException(String.format("%s Failed to lock the state directory: %s", logPrefix, baseDir.getCanonicalPath()));
        }

Copy link
Copy Markdown
Contributor Author

@enothereska enothereska Apr 12, 2017

Choose a reason for hiding this comment

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

Good catch @ijuma . In this case crashing is probably the right thing to do, however I've now caught the exception and propagated further so at least we know why we're crashing.

@enothereska
Copy link
Copy Markdown
Contributor Author

@enothereska
Copy link
Copy Markdown
Contributor Author

@dguy mind having another look at recent change? Thanks.

@asfbot
Copy link
Copy Markdown

asfbot commented Apr 12, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2908/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot
Copy link
Copy Markdown

asfbot commented Apr 12, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2913/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link
Copy Markdown

asfbot commented Apr 12, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2909/
Test PASSed (JDK 8 and Scala 2.12).

Copy link
Copy Markdown
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

LGTM

asfgit pushed a commit that referenced this pull request Apr 12, 2017
Author: Eno Thereska <eno.thereska@gmail.com>

Reviewers: Damian Guy <damian.guy@gmail.com>, Matthias J. Sax <matthias@confluent.io>, Ismael Juma <ismael@juma.me.uk>

Closes #2841 from enothereska/KAFKA-5038
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM, merged to 0.10.2. Please file another PR for trunk.

@enothereska enothereska deleted the KAFKA-5038 branch April 12, 2017 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants