Skip to content

MINOR: rat should depend on processMessages task#13854

Merged
dajac merged 2 commits intoapache:trunkfrom
dajac:fix-rat
Jun 16, 2023
Merged

MINOR: rat should depend on processMessages task#13854
dajac merged 2 commits intoapache:trunkfrom
dajac:fix-rat

Conversation

@dajac
Copy link
Copy Markdown
Member

@dajac dajac commented Jun 14, 2023

This fix the following issue that we occasionally see in builds.

[2023-06-14T11:41:50.769Z] * What went wrong:
[2023-06-14T11:41:50.769Z] A problem was found with the configuration of task ':rat' (type 'RatTask').
[2023-06-14T11:41:50.769Z]   - Gradle detected a problem with the following location: '/home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-13848'.
[2023-06-14T11:41:50.769Z]     
[2023-06-14T11:41:50.769Z]     Reason: Task ':rat' uses this output of task ':clients:processTestMessages' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
[2023-06-14T11:41:50.769Z]     
[2023-06-14T11:41:50.769Z]     Possible solutions:
[2023-06-14T11:41:50.769Z]       1. Declare task ':clients:processTestMessages' as an input of ':rat'.
[2023-06-14T11:41:50.769Z]       2. Declare an explicit dependency on ':clients:processTestMessages' from ':rat' using Task#dependsOn.
[2023-06-14T11:41:50.769Z]       3. Declare an explicit dependency on ':clients:processTestMessages' from ':rat' using Task#mustRunAfter.
[2023-06-14T11:41:50.769Z]     
[2023-06-14T11:41:50.769Z]     Please refer to https://docs.gradle.org/8.1.1/userguide/validation_problems.html#implicit_dependency for more details about this problem.

Validated manually as well:

% ./gradlew rat

> Configure project :
Starting build with version 3.6.0-SNAPSHOT (commit id 874081ca) using Gradle 8.1.1, Java 17 and Scala 2.13.10
Build properties: maxParallelForks=10, maxScalacThreads=8, maxTestRetries=0

> Task :storage:processMessages
MessageGenerator: processed 4 Kafka message JSON files(s).

> Task :raft:processMessages
MessageGenerator: processed 1 Kafka message JSON files(s).

> Task :core:processMessages
MessageGenerator: processed 2 Kafka message JSON files(s).

> Task :group-coordinator:processMessages
MessageGenerator: processed 16 Kafka message JSON files(s).

> Task :streams:processMessages
MessageGenerator: processed 1 Kafka message JSON files(s).

> Task :metadata:processMessages
MessageGenerator: processed 20 Kafka message JSON files(s).

> Task :clients:processMessages
MessageGenerator: processed 146 Kafka message JSON files(s).

> Task :clients:processTestMessages
MessageGenerator: processed 4 Kafka message JSON files(s).

BUILD SUCCESSFUL in 8s

Committer Checklist (excluded from commit message)

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

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Jun 14, 2023

Similar to #13316.

@dajac dajac marked this pull request as ready for review June 14, 2023 18:39
@dajac dajac requested a review from ijuma June 14, 2023 18:39
@divijvaidya
Copy link
Copy Markdown
Member

This problem is annoying and real. Thank you for starting a PR to fix this. Another example build that fails with similar problems: https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-13676/5/pipeline

However, I am wondering if an alternative way to do it is:

Could we create a new root level task called processMessages which will dependOn the following tasks:

:clients:processMessages
:core:processMessages
:group-coordinator:processMessages
:metadata:processMessages
:raft:processMessages
:storage:processMessages
:streams:processMessages

and then, check depends on processMessages
and then, rat already has a dependency on check

This will ensure that rat will depend on any future new subtasks for processMessages. Note that the problem being solved in this PR is slightly different from what was solved in #13316 because over there we wanted each subtask.srcJar to depend on processMessages but now root is a root level task, hence, it can have a root level dependency on each subtasks's :processMessages separately.

Thoughts?

@divijvaidya
Copy link
Copy Markdown
Member

I just encountered another idea.

Just put rat.dependsOn compileJava at

}

You can verify the dependency using:

$ ./gradlew rat --dry-run 

> Configure project :
Starting build with version 3.6.0-SNAPSHOT (commit id cabd6d2e) using Gradle 8.1.1, Java 17 and Scala 2.13.10
Build properties: maxParallelForks=12, maxScalacThreads=8, maxTestRetries=0
:compileJava SKIPPED
:rat SKIPPED

and compileJava depends on processMessages using

$ ./gradlew compileJava --dry-run | grep processMessages
:clients:processMessages SKIPPED
:core:processMessages SKIPPED
:group-coordinator:processMessages SKIPPED
:metadata:processMessages SKIPPED
:raft:processMessages SKIPPED
:storage:processMessages SKIPPED
:streams:processMessages SKIPPED

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Jun 16, 2023

@divijvaidya I just push another approach. It avoids the compilation and the manual dependencies. Let me know what you think.

@divijvaidya
Copy link
Copy Markdown
Member

This is perfect. I was trying a similar approach but somehow my it.tasks wasn't returning the processMessages?!

Can you please verify that it is working fine by running ./gradlew rat --dry-run? It should print the processMessages tasks.

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Jun 16, 2023

% ./gradlew rat --dry-run

> Configure project :
Starting build with version 3.6.0-SNAPSHOT (commit id 55373f1f) using Gradle 8.1.1, Java 17 and Scala 2.13.10
Build properties: maxParallelForks=10, maxScalacThreads=8, maxTestRetries=0
:generator:compileJava SKIPPED
:generator:processResources SKIPPED
:generator:classes SKIPPED
:generator:jar SKIPPED
:clients:processMessages SKIPPED
:clients:processTestMessages SKIPPED
:core:processMessages SKIPPED
:group-coordinator:processMessages SKIPPED
:metadata:processMessages SKIPPED
:raft:processMessages SKIPPED
:storage:processMessages SKIPPED
:streams:processMessages SKIPPED
:rat SKIPPED

Copy link
Copy Markdown
Member

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you for making this change. I looked into our gradle file today and it really needs refactoring since quite a lot of logic can be combined as we did today. Perhaps, for another day.

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Jun 16, 2023

The number if failed tests is scary but none of them are related to this change.

@dajac dajac merged commit 2aa1555 into apache:trunk Jun 16, 2023
@dajac dajac deleted the fix-rat branch June 16, 2023 16:38
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