Skip to content

KAFKA-6878 Switch the order of underlying.init and initInternal#4988

Merged
guozhangwang merged 3 commits intoapache:trunkfrom
tedyu:trunk
May 10, 2018
Merged

KAFKA-6878 Switch the order of underlying.init and initInternal#4988
guozhangwang merged 3 commits intoapache:trunkfrom
tedyu:trunk

Conversation

@tedyu
Copy link
Copy Markdown
Contributor

@tedyu tedyu commented May 9, 2018

This is continuation of #4978.
From Guozhang:

I think to fix this issue, in init we could consider switching the steps of 1 and 2:

initInternal(context);
underlying.init(context, root);
since

volatile boolean open = false;
it should be sufficient. In this case the check on step 3) will fail if underlying.init is not completed and we will throw InvalidStateStoreException.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@guozhangwang
Copy link
Copy Markdown
Contributor

@tedyu could you take a look at other fetch functions in CachingWindowStore as I mentioned in the previous PR?

public void init(final ProcessorContext context, final StateStore root) {
underlying.init(context, root);
initInternal(context);
underlying.init(context, root);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we do this change for CachingSessionStore as well?

@tedyu tedyu changed the title Switch the order of underlying.init and initInternal KAFKA-6878 Switch the order of underlying.init and initInternal May 9, 2018
@tedyu
Copy link
Copy Markdown
Contributor Author

tedyu commented May 9, 2018

Java 8 build was actually successful:

https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/13130/console

@guozhangwang guozhangwang merged commit 8fb5b37 into apache:trunk May 10, 2018
guozhangwang pushed a commit that referenced this pull request May 10, 2018
This is continuation of #4978.
From Guozhang:

I think to fix this issue, in init we could consider switching the steps of 1 and 2:

initInternal(context);
underlying.init(context, root);

since

volatile boolean open = false;

it should be sufficient. In this case the check on step 3) will fail if underlying.init is not completed and we will throw InvalidStateStoreException.

Reviewers: Guozhang Wang <wangguoz@gmail.com>
@guozhangwang
Copy link
Copy Markdown
Contributor

Cherry-picked to 1.1 as well.

ijuma added a commit to ijuma/kafka that referenced this pull request May 11, 2018
…-record-version

* apache-github/trunk:
  KAFKA-6894: Improve err msg when connecting processor with global store (apache#5000)
  KAFKA-6893; Create processors before starting acceptor in SocketServer (apache#4999)
  MINOR: Fix typo in ConsumerRebalanceListener JavaDoc (apache#4996)
  MINOR: Remove deprecated valueTransformer.punctuate (apache#4993)
  MINOR: Update dynamic broker configuration doc for truststore update (apache#4954)
  KAFKA-6870 Concurrency conflicts in SampledStat (apache#4985)
  KAFKA-6361: Fix log divergence between leader and follower after fast leader fail over (apache#4882)
  KAFKA-6813: Remove deprecated APIs in KIP-182, Part II (apache#4976)
  KAFKA-6878 Switch the order of underlying.init and initInternal (apache#4988)
  KAFKA-6299; Fix AdminClient error handling when metadata changes (apache#4295)
  KAFKA-6878: NPE when querying global state store not in READY state (apache#4978)
  KAFKA 6673: Implemented missing override equals method (apache#4745)
  KAFKA-6834: Handle compaction with batches bigger than max.message.bytes (apache#4953)
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
…he#4988)

This is continuation of apache#4978.
From Guozhang:

I think to fix this issue, in init we could consider switching the steps of 1 and 2:

initInternal(context);
underlying.init(context, root);

since

volatile boolean open = false;

it should be sufficient. In this case the check on step 3) will fail if underlying.init is not completed and we will throw InvalidStateStoreException.

Reviewers: Guozhang Wang <wangguoz@gmail.com>
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.

2 participants