Skip to content

MINOR: clarify usage of stateful processor node - #5740

Merged
guozhangwang merged 1 commit into
apache:trunkfrom
vvcephei:fix-stateful-processor-node
Oct 5, 2018
Merged

MINOR: clarify usage of stateful processor node#5740
guozhangwang merged 1 commit into
apache:trunkfrom
vvcephei:fix-stateful-processor-node

Conversation

@vvcephei

@vvcephei vvcephei commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

In recent PRs, we have been confused about the proper usage of
StatefulProcessorNode (#5731 , #5737 )

This change disambiguates it.

Committer Checklist (excluded from commit message)

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

@vvcephei vvcephei left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@guozhangwang @mjsax @bbejeck ,

In light of the recent confusion about this class, I was thinking this change might help avoid it in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Having a separate method for getStatefulProcessorNode was unnecessary, so I've just inlined it to call the constructor directly instead. Perhaps that method did extra work in the past.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the core change. Instead of having one constructor and a contract that callers should specify either names or a store builder and set the other to null is confusing. Instead, we'll have two constructors, one for names and the other for the store builder.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is an internal class, and the constructor is quite simple. Having a builder for it seems like extra code for no real gain. Plus, it makes it more difficult for callers to tell what arguments are expected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The compiler can't tell which constructor to call, so we cast null to pick one. It doesn't matter which, since this argument is never evaluated.

@vvcephei

vvcephei commented Oct 4, 2018

Copy link
Copy Markdown
Contributor Author

Failure unrelated. (spotbugs couldn't write to some local socket?)

Retest this, please.

@bbejeck bbejeck left a comment

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.

Thanks @vvcephei.

Having 2 constructors does clear things up. Just one comment about adding some Javadoc on the two constructors.

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.

I agree having two separate constructors clears usage of StatefulProcessorNode up.

Maybe we could add some Javadoc comments that the constructor taking the StateStore parameter is for stateful operations that create a state store.
And the constructor taking storeNames is for stateful DSL methods process, transform and the other transform variants where the state store is already created (with a StreamsBuilder.addStateStore call) , and we pass in the names so the Processor can access the stores.

@guozhangwang

Copy link
Copy Markdown
Contributor

Waiting for jenkins to pass.

@vvcephei

vvcephei commented Oct 4, 2018

Copy link
Copy Markdown
Contributor Author

Major hiccough in the core integration tests.

Retest this, please.

@mjsax mjsax left a comment

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.

LGTM.

Very nice cleanup @vvcephei!

@guozhangwang

Copy link
Copy Markdown
Contributor

@vvcephei could you rebase please?

@vvcephei

vvcephei commented Oct 5, 2018

Copy link
Copy Markdown
Contributor Author

@guozhangwang, It is rebased.

@guozhangwang
guozhangwang merged commit 1bc620d into apache:trunk Oct 5, 2018
guozhangwang pushed a commit that referenced this pull request Oct 5, 2018
In recent PRs, we have been confused about the proper usage of
StatefulProcessorNode (#5731 , #5737 )

This change disambiguates it.

Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>
@guozhangwang

Copy link
Copy Markdown
Contributor

Cherry-picked to 2.1 as well.

@vvcephei

vvcephei commented Oct 5, 2018

Copy link
Copy Markdown
Contributor Author

Thank you!

@vvcephei
vvcephei deleted the fix-stateful-processor-node branch October 5, 2018 17:25
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
In recent PRs, we have been confused about the proper usage of
StatefulProcessorNode (apache#5731 , apache#5737 )

This change disambiguates it.

Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>
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.

4 participants