Skip to content

Comments

Move JDK11 ITs to cron stage#13075

Merged
gianm merged 4 commits intoapache:masterfrom
abhishekagarwal87:less_travis_tests
Sep 20, 2022
Merged

Move JDK11 ITs to cron stage#13075
gianm merged 4 commits intoapache:masterfrom
abhishekagarwal87:less_travis_tests

Conversation

@abhishekagarwal87
Copy link
Contributor

@abhishekagarwal87 abhishekagarwal87 commented Sep 12, 2022

This PR moves some of the ITs for JRE 11 runtime to the cron stage. The reason is simple - we have a lot of tests and our PR checks take too long to finish. This would improve the situation slightly. I have kept the following ITs around

  • query integration test (so we at least check the whole ingestion and querying end-to-end on runtime 11)
  • security integration test, leadership integration test, query integration test (mariaDB) - I kept these around since they involve another service like a zookeeper. There are no unit tests with the MariaDB driver. The same goes for TLSCertificateChecker.

This PR has:

  • been self-reviewed.
  • been tested in a test Druid cluster.

@abhishekagarwal87 abhishekagarwal87 marked this pull request as ready for review September 12, 2022 14:37
@gianm
Copy link
Contributor

gianm commented Sep 13, 2022

When do the cron tests run and how can we see the results? Is there a way to run them on-demand on release branches? (Running "extra bonus" tests on release candidates seems like the best use of them.)

Please forgive me for these basic questions; I'm just not familiar with how the cron tests work.


- <<: *integration_tests
name: "(Compile=openjdk8, Run=openjdk11) other integration test"
jdk: openjdk8
Copy link
Contributor

@kfaraz kfaraz Sep 13, 2022

Choose a reason for hiding this comment

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

Is this correct? This entry and all the other modified ones seem to be for jdk8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. druid gets compiled and built with JDK 8 but then tests are run on JRE 11.

@abhishekagarwal87
Copy link
Contributor Author

When do the cron tests run and how can we see the results? Is there a way to run them on-demand on release branches? (Running "extra bonus" tests on release candidates seems like the best use of them.)

Please forgive me for these basic questions; I'm just not familiar with how the cron tests work.

I forgive you :)
The cron tests run daily on the master branch. It can be configured in the https://app.travis-ci.com/github/apache/druid/settings.
The notifications for success/failures are sent to dev@druid.apache.org. For some reason, I don't see a daily email. If you search "[CRON]" in your email, you can see these notifications.

I don't see a way to run them on-demand on release branches. We can however make a slight change in .travis.yml so the cron stage becomes a regular third stage.

@abhishekagarwal87
Copy link
Contributor Author

I could also set a condition such that cron jobs would always run on master and release branches but not on pull requests.

@gianm
Copy link
Contributor

gianm commented Sep 13, 2022

I could also set a condition such that cron jobs would always run on master and release branches but not on pull requests.

Seems like running them automatically on release branches would be good; that way, it makes life easier for release managers. They would be able to backport things without waiting for CI to pass on each PR, since they could look at the release-branch results after backports are done. That also ensures we run the full test suite on every release.

@gianm
Copy link
Contributor

gianm commented Sep 13, 2022

I could also set a condition such that cron jobs would always run on master and release branches but not on pull requests.

Is this something that would be done in .travis.yml in this PR? If so, it sounds good to me, for the reasons in my prior comment.

@gianm
Copy link
Contributor

gianm commented Sep 13, 2022

Final note: please also update distribution/asf-release-process-guide.md so release managers know to check these extra tests.

@abhishekagarwal87
Copy link
Contributor Author

@gianm - I have made the changes you suggested. PTAL.

The only additions to the release branch after branching should be bug fixes, which should be back-ported from the master branch, via a second PR or a cherry-pick, not with a direct PR to the release branch.

Once all issues and PRs that are still tagged with the release milestone have been merged, closed, or removed from the milestone, the next step is to put together a release candidate.
Release manager must also ensure that CI is passing successfully on the release branch. Since CI on branch can contain additional tests such as ITs for different JVM flavours. (Note that CI is sometimes flaky for older branches).
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please include some instruction about how to ensure that CI passes successfully on the branch? Like, a link or a description of what to click where?

.travis.yml Outdated
# 3. cron
#
# The cron type only runs jobs that are marked with stage cron.
# The cron type only runs jobs that are marked with stage cron. The ron stage also runs alongside
Copy link
Contributor

Choose a reason for hiding this comment

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

"cron stage" (spelling)

if: type != cron
- name: cron
if: type = cron
if: type = cron OR (type != pull_request AND branch != master)
Copy link
Contributor

Choose a reason for hiding this comment

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

https://docs.travis-ci.com/user/conditions-v1 says the possible type are push, pull_request, api, cron. So these tests will now additionally run for nonmaster branches if the event type is push or api. I guess api is for manually-triggered tests on branches. What's push for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am guessing but it must be triggered for any commit that is pushed to the branch. I could not explicit documentation but that definition seems most plausible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's try this and see if it does what we want.

@gianm gianm merged commit 455b074 into apache:master Sep 20, 2022
@kfaraz kfaraz added this to the 25.0 milestone Nov 22, 2022
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.

3 participants