Skip to content

KAFKA-8934: Create version file during build for Streams#7397

Merged
guozhangwang merged 2 commits intoapache:trunkfrom
cadonna:AK8934-copy_version_file_to_streams
Sep 27, 2019
Merged

KAFKA-8934: Create version file during build for Streams#7397
guozhangwang merged 2 commits intoapache:trunkfrom
cadonna:AK8934-copy_version_file_to_streams

Conversation

@cadonna
Copy link
Copy Markdown
Member

@cadonna cadonna commented Sep 26, 2019 •
edited
Loading

The Kafka Clients library includes a version file that
contains its version and Git commit ID.
Since Kafka Streams wants to expose version and commit ID
in the metrics it needs to read the version file.
To enable the users to check during runtime for
version mismatches between the Streams library and the Clients
library, a version file is also created for Streams during build
time and during runtime only the Streams version file is read.
If Streams would read the Clients' version file during runtime, it
would read a wrong version and commit ID if the libraries were
built from repositories in different states.

Committer Checklist (excluded from commit message)

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

@cadonna
Copy Clients' version file during build to Streams
a5d8203 
The Kafka Clients library includes a version file that
contains its version and Git commit ID.
Since Kafka Streams wants to expose version and commit ID
in the metrics it needs to read the version file.
To enable the users to check during runtime for
version mismatches between the Streams library and the Clients
library, the version file is copied from Clients during build
time and during runtime only the Streams version file is read.
If Streams would read Clients' version file during runtime, it
would read a wrong version and commit ID if the libraries where
not build from repositories in different states.
@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Sep 26, 2019

Call for review @guozhangwang @vvcephei

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Sep 26, 2019 •
edited
Loading

JDK 8/Scala 2.11 failed with the following error:

There is insufficient memory for the Java Runtime Environment to continue.

JDK 11/Scala2.13 failed with the following failing tests:

org.scalatest.exceptions.TestFailedException: Timed out before consuming expected 2700 records. The number consumed was 1980.

Retest this, please

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Sep 26, 2019

Retest this, please

@vvcephei
Copy link
Copy Markdown
Contributor

vvcephei commented Sep 26, 2019

Hey, thanks for this, @cadonna ! I've been bitten by this a few times as well.

Instead of copying the version file from the clients module, can we just generate our own the same way they do? It just seems a little cleaner that way, so there's not the extra dependency on an "implementation detail" of the clients build.

Thanks again!
-John

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Sep 26, 2019

Thanks @cadonna for this PR.
I've been bitten by this before as well. I agree with @vvcephei that we should create our own file as well.

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Sep 26, 2019

@vvcephei @bbejeck Here you are ;-)

@cadonna cadonna changed the title Copy Clients' version file to Streams during build KAFKA-8934: Create version file during build for Streams Sep 26, 2019
vvcephei
vvcephei reviewed Sep 26, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, @cadonna ! Did you also want to add a log message or something to log the information on startup? Or is this just preliminary to adding metrics/logs later?

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Sep 26, 2019

Yes, this is just preliminary. This information will be exposed in client-level metrics.

vvcephei
vvcephei approved these changes Sep 26, 2019
View reviewed changes
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.

Thanks, @cadonna !

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Sep 27, 2019

Retest this, please

1 similar comment
@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Sep 27, 2019

Retest this, please

@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Sep 27, 2019

@cadonna could you run the whole unit test suite locally? Jenkins seems not happy atm.

guozhangwang
guozhangwang reviewed Sep 27, 2019
View reviewed changes
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.

One minor comment.

Comment thread build.gradle
duplicatesStrategy 'exclude'
}

task determineCommitId {
Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang Sep 27, 2019

Choose a reason for hiding this comment

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

The determineCommitId logic is the same for both client and streams, I'm wondering if we can make it a global task that both can rely on (I'm not an expert of gradle, just wondering).

Copy link
Copy Markdown
Member Author

@cadonna cadonna Sep 27, 2019

Choose a reason for hiding this comment

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

I also had the same thought and I am also not a gradle expert. I would leave the duplicate code as it is for now to have a chance to get client-level metrics into the release and fix this afterwards.

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Sep 27, 2019

@guozhangwang: all tests pass locally

@guozhangwang guozhangwang merged commit 863210d into apache:trunk Sep 27, 2019
ijuma added a commit to confluentinc/kafka that referenced this pull request Sep 29, 2019
@ijuma
Sync from apache/kafka (28 September 2019)
2715861 
Conflicts:
* .gitignore: addition of clients/src/generated-test was near
local additions for support-metrics.
* checkstyle/suppressions.xml: upstream refactoring of exclusions
for generator were near the local changes for support-metrics.
* gradle.properties: scala version bump caused a minor conflict
due to the kafka version change locally.
gradle/dependencies.gradle: bcpkix version bump was near avro
additions in the local version.

* apache-github/trunk: (49 commits)
  KAFKA-8471: Replace control requests/responses with automated protocol (apache#7353)
  MINOR: Don't generate unnecessary strings for debug logging in FetchSessionHandler (apache#7394)
  MINOR:fixed typo and removed outdated varilable name (apache#7402)
  KAFKA-8934: Create version file during build for Streams (apache#7397)
  KAFKA-8319: Make KafkaStreamsTest a non-integration test class (apache#7382)
  KAFKA-6883: Add toUpperCase support to sasl.kerberos.principal.to.local rule (KIP-309)
  KAFKA-8907; Return topic configs in CreateTopics response (KIP-525) (apache#7380)
  MINOR: Address review comments for KIP-504 authorizer changes (apache#7379)
  MINOR: add versioning to request and response headers (apache#7372)
  KAFKA-7273: Extend Connect Converter to support headers (apache#6362)
  MINOR: improve the Kafka RPC code generator (apache#7340)
  MINOR: Improve the org.apache.kafka.common.protocol code (apache#7344)
  KAFKA-8880: Docs on upgrade-guide (apache#7385)
  KAFKA-8179: do not suspend standby tasks during rebalance (apache#7321)
  KAFKA-8580: Compute RocksDB metrics (apache#7263)
  KAFKA-8880: Add overloaded function of Consumer.committed (apache#7304)
  HOTFIX: fix Kafka Streams upgrade note for broker backward compatibility (apache#7363)
  KAFKA-8848; Update system tests to use new AclAuthorizer (apache#7374)
  MINOR: remove unnecessary null check (apache#7299)
  KAFKA-6958: Overload methods for group and windowed stream to allow to name operation name using the new Named class (apache#6413)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vvcephei vvcephei vvcephei approved these changes

@guozhangwang guozhangwang guozhangwang approved these changes

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cadonna @vvcephei @bbejeck @guozhangwang