Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ISSUE #2640: BP-43 Migrate to gradle all other tests #2812

Merged
merged 1 commit into from
Oct 20, 2021
Merged

ISSUE #2640: BP-43 Migrate to gradle all other tests #2812

merged 1 commit into from
Oct 20, 2021

Conversation

pkumar-singh
Copy link
Member

@pkumar-singh pkumar-singh commented Oct 1, 2021 •
edited
Loading

Motivation

  • Moving all the tests to gradle.
  • Rearranging github action files to run all tests but only once.
  • Splitting tests in many github action files for equitable distribution of tests by time and types.
  • Making changes in the gradle dependencies to pull exactly same dependencies as maven. Goal is to be consistent with maven even though maven could be pulling in redundant dependencies.

Changes

  • Do not change code.
  • Only changes in gradle files.
  • Do not change licenses, that way it sort of ensures that gradle is pulling in same dependencies as maven

How to revert.

Just restore the github action files from before.

Master Issue: #2640

@pkumar-singh pkumar-singh changed the title Migrate to gradle all other tests {Draft}Migrate to gradle all other tests Oct 1, 2021
@pkumar-singh pkumar-singh force-pushed the migrate_to_gradle_all_other_tests branch from 840801f to cfaeb4c Compare October 1, 2021 17:58
@pkumar-singh pkumar-singh marked this pull request as draft October 1, 2021 17:59
@pkumar-singh pkumar-singh force-pushed the migrate_to_gradle_all_other_tests branch from cfaeb4c to 46770d2 Compare October 1, 2021 18:06
@pradeepbn pradeepbn mentioned this pull request Oct 6, 2021
Closed
@pkumar-singh pkumar-singh force-pushed the migrate_to_gradle_all_other_tests branch 6 times, most recently from bff401a to 7bd87ce Compare October 16, 2021 01:43
@pkumar-singh pkumar-singh marked this pull request as ready for review October 18, 2021 17:02
@pkumar-singh pkumar-singh changed the title {Draft}Migrate to gradle all other tests ISSUE #2640: BP-43 Migrate to gradle all other tests Oct 18, 2021
Migrate bookkeepr-server:test to gradle run unit tests excepts org.ap…
ea86624 
…ache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls.
@pkumar-singh pkumar-singh force-pushed the migrate_to_gradle_all_other_tests branch from 7bd87ce to ea86624 Compare October 18, 2021 20:14
nicoloboschi
nicoloboschi approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

LGTM, really great work!

Did you try to compare dist artifact built with maven and gradle?
My mainly concern is we can mismatch a dependency's version or similar, and the final jars will be unintentionally different

eolivelli
eolivelli reviewed Oct 19, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Overall +1

I left one comment about Apache-RAT check, that is very important for us

.github/workflows/pr-validation.yml Show resolved Hide resolved
@eolivelli
Copy link
Contributor

Did you try to compare dist artifact built with maven and gradle?
My mainly concern is we can mismatch a dependency's version or similar, and the final jars will be unintentionally different

@nicoloboschi maybe it is a good idea if you can double check by yourself

@pkumar-singh
Copy link
Member Author

LGTM, really great work!

Did you try to compare dist artifact built with maven and gradle? My mainly concern is we can mismatch a dependency's version or similar, and the final jars will be unintentionally different

Absolutely.!
Basically as part of PR build following script runs.
dev/check-all-licenses-gradle
What it does is. Untar the tar ball created by gradle. That tar ball contains all bookkeeper jars as well as dependencies jars.

Now script matches each jar in the tar ball with the one mentioned here
https://github.com/apache/bookkeeper/blob/master/bookkeeper-dist/src/main/resources/LICENSE-server.bin.txt

Here https://github.com/apache/bookkeeper/blob/master/bookkeeper-dist/src/main/resources/LICENSE-all.bin.txt

And here https://github.com/apache/bookkeeper/blob/master/bookkeeper-dist/src/main/resources/NOTICE-bkctl.bin.txt

Script also ensure that tar ball does not pack any extra jar, in addition to ensuring exact version with the files mentioned above.

Moreover, I have also manually untared the tar ball generated by gradle and maven and check one by one manually to verify they pack same dependencies.

hsaputra
hsaputra approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@hsaputra hsaputra left a comment

Choose a reason for hiding this comment

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

LGTM +1

@hsaputra
Copy link
Contributor

Will merge this EOD PST if no more objection or comment. Thanks!

eolivelli
eolivelli approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

@hsaputra hsaputra added this to the 4.15.0 milestone Oct 20, 2021
@hsaputra hsaputra merged commit 3dd671c into apache:master Oct 20, 2021
hsaputra pushed a commit that referenced this pull request Oct 29, 2021
@nicoloboschi
Gradle build - some submodules are skipped
98ddf81 
### Motivation

In order to complete migration to Gradle we must build all the subprojects.

### Changes

- Enabled `sh` integration tests with gradle, located in `tests/scripts/src/test/bash/gradle`
- Added these modules to the build
    - `bookkeeper-http:servlet-http-server` 
    - `metadata-drivers:etcd`
    - `tests:backward-compat:*`
    - `tests:shaded:*`
    - `stream:bk-grpc-name-resolver`
- DL shading process is now performed (before it didn't build any jar)
- Groovy tests (`tests:backward-compat:*`) now are triggered by the build/tests itself; with Maven, there is a "runner" project (`tests/integration-tests-base-groovy`); in Gradle is useless so it is skipped


### Test

- Both `bin/bookkeper standalone` and `bin/bookkeper_gradle standalone` work locally
- Tests are passing locally  

Master Issue: #2849



Reviewers: Henry Saputra <hsaputra@apache.org>, Prashant Kumar <None>

This closes #2850 from nicoloboschi/fix/2849/gradle and squashes the following commits:

00b49f4 [Nicolò Boschi] Fix common_gradle.sh regex
bd739fd [Nicolò Boschi] fix sh tests
43230ba [Nicolò Boschi] revert sh files. Avoid to modify maven files, create gradle versions to faciltate migration
d1f95e4 [Nicolò Boschi] fix shaded deps
bcab40d [Nicolò Boschi] fix build
5fd0341 [Nicolò Boschi] fix build
0082e0e [Nicolò Boschi] fix build
2c32ac1 [Nicolò Boschi] fixes
3bc0b26 [Nicolò Boschi] bookkeeper-server-shaded-tests
ba89132 [Nicolò Boschi] shaded tests
6d39e33 [Nicolò Boschi] sh tests
e0032bc [Nicolò Boschi] actually run arquillian groovy tests
08dcc39 [Nicolò Boschi] backwards
2361f79 [Nicolò Boschi] hierarchical-ledger-manager
8388e11 [Nicolò Boschi] current-server-old-clients
6a24344 [Nicolò Boschi] bc-non-fips
2faca01 [Nicolò Boschi] bk-grpc-name-resolver
991bc11 [Nicolò Boschi] servlet-http-server
675ef7b [Nicolò Boschi] etcd
b1d5e14 [ZhangJian He] A empty implement in EtcdLedgerManagerFactory to let the project can compile (#2845)
bd5c50b [shustsud] Add error handling to readLedgerMetadata in over-replicated ledger GC (#2844)
746f9f6 [Matteo Merli] Remove direct ZK access for Auditor (#2842)
4117200 [ZhangJian He] the compare should be >= instead of > (#2782)
14ef56f [Prashant Kumar] BookieId can not be cast to BookieSocketAddress (#2843)
e10f3fe [ZhangJian He] Forget to close preAllocator log on shutdown (#2819)
53954ca [shustsud] Add ensemble check to over-replicated ledger GC (#2813)
919fdd3 [Prashant Kumar] Issue:2840 Create bookie shellscript for gradle (#2841)
031d168 [gaozhangmin] fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver (#2788)
3dd671c [Prashant Kumar] Migrate bookkeepr-server:test to gradle run unit tests excepts org.apache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls. (#2812)
f6903b8 [Jack Vanlightly] BP-44 USE metrics
a4afaa4 [Matteo Merli] Eliminate direct ZK access in ScanAndCompareGarbageCollector (#2833)
a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (#2832)
148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (#2821)
4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (#2794)
a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (#2816)
0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (#2793)
594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (#2796)
354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (#2792)
e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (#2779)
883231e [pradeepbn] Building bookkeeper with gradle on java11
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
@pkumar-singh
Migrate bookkeepr-server:test to gradle run unit tests excepts org.ap…
1ef5e20 
…ache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls. (apache#2812)

Co-authored-by: Prashant Kumar <prashantk@splunk.com>
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
@nicoloboschi
Gradle build - some submodules are skipped
ff821cb 
### Motivation

In order to complete migration to Gradle we must build all the subprojects.

### Changes

- Enabled `sh` integration tests with gradle, located in `tests/scripts/src/test/bash/gradle`
- Added these modules to the build
    - `bookkeeper-http:servlet-http-server` 
    - `metadata-drivers:etcd`
    - `tests:backward-compat:*`
    - `tests:shaded:*`
    - `stream:bk-grpc-name-resolver`
- DL shading process is now performed (before it didn't build any jar)
- Groovy tests (`tests:backward-compat:*`) now are triggered by the build/tests itself; with Maven, there is a "runner" project (`tests/integration-tests-base-groovy`); in Gradle is useless so it is skipped


### Test

- Both `bin/bookkeper standalone` and `bin/bookkeper_gradle standalone` work locally
- Tests are passing locally  

Master Issue: apache#2849



Reviewers: Henry Saputra <hsaputra@apache.org>, Prashant Kumar <None>

This closes apache#2850 from nicoloboschi/fix/2849/gradle and squashes the following commits:

00b49f4 [Nicolò Boschi] Fix common_gradle.sh regex
bd739fd [Nicolò Boschi] fix sh tests
43230ba [Nicolò Boschi] revert sh files. Avoid to modify maven files, create gradle versions to faciltate migration
d1f95e4 [Nicolò Boschi] fix shaded deps
bcab40d [Nicolò Boschi] fix build
5fd0341 [Nicolò Boschi] fix build
0082e0e [Nicolò Boschi] fix build
2c32ac1 [Nicolò Boschi] fixes
3bc0b26 [Nicolò Boschi] bookkeeper-server-shaded-tests
ba89132 [Nicolò Boschi] shaded tests
6d39e33 [Nicolò Boschi] sh tests
e0032bc [Nicolò Boschi] actually run arquillian groovy tests
08dcc39 [Nicolò Boschi] backwards
2361f79 [Nicolò Boschi] hierarchical-ledger-manager
8388e11 [Nicolò Boschi] current-server-old-clients
6a24344 [Nicolò Boschi] bc-non-fips
2faca01 [Nicolò Boschi] bk-grpc-name-resolver
991bc11 [Nicolò Boschi] servlet-http-server
675ef7b [Nicolò Boschi] etcd
b1d5e14 [ZhangJian He] A empty implement in EtcdLedgerManagerFactory to let the project can compile (apache#2845)
bd5c50b [shustsud] Add error handling to readLedgerMetadata in over-replicated ledger GC (apache#2844)
746f9f6 [Matteo Merli] Remove direct ZK access for Auditor (apache#2842)
4117200 [ZhangJian He] the compare should be >= instead of > (apache#2782)
14ef56f [Prashant Kumar] BookieId can not be cast to BookieSocketAddress (apache#2843)
e10f3fe [ZhangJian He] Forget to close preAllocator log on shutdown (apache#2819)
53954ca [shustsud] Add ensemble check to over-replicated ledger GC (apache#2813)
919fdd3 [Prashant Kumar] Issue:2840 Create bookie shellscript for gradle (apache#2841)
031d168 [gaozhangmin] fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver (apache#2788)
3dd671c [Prashant Kumar] Migrate bookkeepr-server:test to gradle run unit tests excepts org.apache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls. (apache#2812)
f6903b8 [Jack Vanlightly] BP-44 USE metrics
a4afaa4 [Matteo Merli] Eliminate direct ZK access in ScanAndCompareGarbageCollector (apache#2833)
a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (apache#2832)
148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (apache#2821)
4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (apache#2794)
a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (apache#2816)
0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (apache#2793)
594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (apache#2796)
354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (apache#2792)
e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (apache#2779)
883231e [pradeepbn] Building bookkeeper with gradle on java11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@eolivelli eolivelli eolivelli approved these changes

@hsaputra hsaputra hsaputra approved these changes

@dlg99 dlg99 Awaiting requested review from dlg99

@ivankelly ivankelly Awaiting requested review from ivankelly

Assignees

@pkumar-singh pkumar-singh

Labels
area/tests release/4.15.0 type/feature
Projects
None yet
Milestone
4.15.0
Development

Successfully merging this pull request may close these issues.
4 participants
@pkumar-singh @eolivelli @hsaputra @nicoloboschi