Skip to content

KIP-363 Make FunctionConversions deprecated#5562

Merged
guozhangwang merged 1 commit intoapache:trunkfrom
joan38:private-func-conv
Sep 19, 2018
Merged

KIP-363 Make FunctionConversions deprecated#5562
guozhangwang merged 1 commit intoapache:trunkfrom
joan38:private-func-conv

Conversation

@joan38
Copy link
Copy Markdown
Contributor

@joan38 joan38 commented Aug 23, 2018

As pointed out in this comment #5539 (comment) the object FunctionConversions is only of internal use and therefore should be private to the lib only so that we can do changes without going through KIP like this one.

KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-363%3A+Make+FunctionConversions+private

@guozhangwang @vvcephei @mjsax @debasishg and whoever wants to join the party 😄

@joan38 joan38 changed the title Make FunctionConversions private KIP-363 Make FunctionConversions private Aug 23, 2018
@joan38 joan38 force-pushed the private-func-conv branch from b715601 to 32a5b31 Compare August 24, 2018 00:42
@joan38
Copy link
Copy Markdown
Contributor Author

joan38 commented Aug 25, 2018

We are waiting for the min of 72h of the KIP vote to finish:
http://mail-archives.apache.org/mod_mbox/kafka-dev/201808.mbox/%3CCAJYfrs3GOrFrY9K3wToyg+Pj-AXtc=RZvvCWTH9fmWVjsEYs_w@mail.gmail.com%3E

Which means we can merge this after 26 Aug 2018 18:24:56 GMT

Can we label this streams @mjsax ?

Thanks

@debasishg
Copy link
Copy Markdown
Contributor

👍

@joan38
Copy link
Copy Markdown
Contributor Author

joan38 commented Aug 27, 2018

@guozhangwang This is all good to merge

@mjsax mjsax added the streams label Sep 7, 2018
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Sep 7, 2018

@joan38 I just replied to the KIP thread -- also note, that the vote did not pass yet and the KIP is not accepted yet. It's required to have 3 binding votes (so far only Guozhang gave binding +1).

Also, can you update this PR accordingly? Thanks a lot.

@joan38 joan38 changed the title KIP-363 Make FunctionConversions private KIP-363 Make FunctionConversions deprecated Sep 14, 2018
@joan38 joan38 force-pushed the private-func-conv branch 3 times, most recently from 221e5ed to 345ae3b Compare September 14, 2018 19:42
@joan38
Copy link
Copy Markdown
Contributor Author

joan38 commented Sep 15, 2018

12:40:15 * What went wrong:
12:40:15 Execution failed for task ':streams:streams-scala:spotbugsMain'.
12:40:15 > SpotBugs rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/kafka-pr-jdk10-scala2.12/streams/streams-scala/build/reports/spotbugs/main.xml

Do you know what this means?

@guozhangwang
Copy link
Copy Markdown
Contributor

@joan38 spotbugs was added recently, you can run ./gradlew spotbugsMain spotbugsTest -x test (stated in README.md) locally to see the results.

@joan38
Copy link
Copy Markdown
Contributor Author

joan38 commented Sep 17, 2018

Thanks @guozhangwang
I get:

Bug type EQ_UNUSUAL (click for details) In class org.apache.kafka.streams.scala.FunctionsCompatConversions$AggregatorFromFunctionIn method org.apache.kafka.streams.scala.FunctionsCompatConversions$AggregatorFromFunction.equals(Object)At FunctionsCompatConversions.scala:[line 88]
--
EQ_UNUSUAL: Unusual equals method
This class doesn't do any of the patterns we recognize for checking that the type of the argument is compatible with the type of the this object. There might not be anything wrong with this code, but it is worth reviewing.

screen shot 2018-09-17 at 08 40 18

Is it possible to disable this since it's a copy and past from the other class?

@vvcephei
Copy link
Copy Markdown
Contributor

@joan38 . Hmm. I'm not sure why it's running for that one class. Perhaps the other class is already excluded, and this one doesn't match the pattern?

The exclusions are here: gradle/spotbugs-exclude.xml

@joan38
Copy link
Copy Markdown
Contributor Author

joan38 commented Sep 17, 2018

@vvcephei you are right, it is excluded.
I will add this one also to the exclusion.

Other than that are we happy with the change?

Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Hey @joan38, yep, assuming the tests are passing, this LGTM. Thanks!

@guozhangwang
Copy link
Copy Markdown
Contributor

spotbug still fails in jenkins:

04:43:17 Execution failed for task ':streams:streams-scala:spotbugsMain'.

@joan38
Copy link
Copy Markdown
Contributor Author

joan38 commented Sep 18, 2018

It's now ready to merge

Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

LGTM!

@guozhangwang guozhangwang merged commit af80dcc into apache:trunk Sep 19, 2018
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Reviewers: John Roesler <vvcephei@users.noreply.github.com>, Guozhang Wang <wangguoz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants