You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2021/12/13 15:57:28 UTC
[GitHub] [bookkeeper] nicoloboschi opened a new pull request #2937: Gradle - Enable backward-compatibility tests
nicoloboschi opened a new pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937
### Motivation
Integration tests under `backward-compat` do not work neither with Gradle and Maven.
For Maven, they are not triggered at all
For Gradle, they run and fail because in Gradle the docker images used inside the tests are not built at all.
### Changes
* Build docker images under `tests/docker-images`
* Fix some tests failing due to BookieId migration
* Fix `tests/backward-compat/upgrade/src/test/groovy/org/apache/bookkeeper/tests/backwardcompat/TestCompatUpgrade.groovy` test which needs tests running in a specific order
* Fix `MavenClassLoader`removing the need to have the current SNAPSHOT version jars in the local repository. Now it uses the distribution tar.gz
* Add these tests in GH actions
Some important notes:
* I haven't found the date where tests were not being run anymore but I think is more than 1 year ago, at least since BP-41
* I kept Arquillian Cube as docker container runner since there's no real need to migrate to testcontainers
* **These tests only work on Linux** because they relies on `Host` docker network which is only available on Linux
* I haven't fixed Maven execution since we are already using Gradle in CI
Master Issue: #2930
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli merged pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#discussion_r773706994
##########
File path: tests/docker-images/statestore-image/image_builder.sh
##########
@@ -18,22 +18,34 @@
# * limitations under the License.
# */
+IMAGE_NAME=apachebookkeeper/bookkeeper-current:latest
+if [[ "$(docker images -q $IMAGE_NAME 2> /dev/null)" != "" ]]; then
+ echo "reusing local image: $IMAGE_NAME"
+ exit 0
Review comment:
I added a env property to force the rebuilding of the images. By default is false since it's uncommon you want to refresh those images
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#discussion_r773383043
##########
File path: tests/backward-compat/current-server-old-clients/src/test/groovy/org/apache/bookkeeper/tests/backwardcompat/AbstractClientFencingTest.groovy
##########
@@ -0,0 +1,97 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package org.apache.bookkeeper.tests.backwardcompat
+
+import com.github.dockerjava.api.DockerClient
+import org.apache.bookkeeper.tests.integration.utils.BookKeeperClusterUtils
+import org.apache.bookkeeper.tests.integration.utils.MavenClassLoader
+import org.jboss.arquillian.test.api.ArquillianResource
+import org.junit.Assert
+import org.junit.Before
+
+abstract class AbstractClientFencingTest {
+ protected static byte[] PASSWD = "foobar".getBytes()
+
+ @ArquillianResource
+ protected DockerClient docker
+
+ protected String currentVersion = BookKeeperClusterUtils.CURRENT_VERSION
+
+ @Before
+ public void before() throws Exception {
+ Assert.assertTrue(BookKeeperClusterUtils.stopAllBookies(docker))
+ // First test to run, formats metadata and bookies
+ if (BookKeeperClusterUtils.metadataFormatIfNeeded(docker, currentVersion)) {
+ BookKeeperClusterUtils.formatAllBookies(docker, currentVersion)
+ }
+ // If already started, this has no effect
+ Assert.assertTrue(BookKeeperClusterUtils.startAllBookiesWithVersion(docker, currentVersion))
+ }
+
+ protected void testFencingOldClient(String oldClientVersion, String fencingVersion) {
+ String zookeeper = BookKeeperClusterUtils.zookeeperConnectString(docker)
+
+ def oldCL = MavenClassLoader.forBookKeeperVersion(oldClientVersion)
+ def oldBK = oldCL.newBookKeeper(zookeeper)
+ def fencingCL = MavenClassLoader.forBookKeeperVersion(fencingVersion)
+ def fencingBK = fencingCL.newBookKeeper(zookeeper)
+
+ try {
+ def numEntries = 5
+ def ledger0 = oldBK.createLedger(3, 2,
+ oldCL.digestType("CRC32"),
+ PASSWD)
+ for (int i = 0; i < numEntries; i++) {
+ ledger0.addEntry(("foobar" + i).getBytes())
+ }
+ ledger0.close()
+
+
+ def ledger1 = fencingBK.openLedger(ledger0.getId(), fencingCL.digestType("CRC32"), PASSWD)
+
+ // cannot write any more
+ try {
+ ledger0.addEntry("shouldn't work".getBytes())
+ Assert.fail("Shouldn't have been able to add any more")
+ } catch (Exception e) {
Review comment:
Why not just catch BKLedgerClosedException
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#discussion_r773706349
##########
File path: tests/backward-compat/current-server-old-clients/src/test/groovy/org/apache/bookkeeper/tests/backwardcompat/AbstractClientFencingTest.groovy
##########
@@ -0,0 +1,97 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package org.apache.bookkeeper.tests.backwardcompat
+
+import com.github.dockerjava.api.DockerClient
+import org.apache.bookkeeper.tests.integration.utils.BookKeeperClusterUtils
+import org.apache.bookkeeper.tests.integration.utils.MavenClassLoader
+import org.jboss.arquillian.test.api.ArquillianResource
+import org.junit.Assert
+import org.junit.Before
+
+abstract class AbstractClientFencingTest {
+ protected static byte[] PASSWD = "foobar".getBytes()
+
+ @ArquillianResource
+ protected DockerClient docker
+
+ protected String currentVersion = BookKeeperClusterUtils.CURRENT_VERSION
+
+ @Before
+ public void before() throws Exception {
+ Assert.assertTrue(BookKeeperClusterUtils.stopAllBookies(docker))
+ // First test to run, formats metadata and bookies
+ if (BookKeeperClusterUtils.metadataFormatIfNeeded(docker, currentVersion)) {
+ BookKeeperClusterUtils.formatAllBookies(docker, currentVersion)
+ }
+ // If already started, this has no effect
+ Assert.assertTrue(BookKeeperClusterUtils.startAllBookiesWithVersion(docker, currentVersion))
+ }
+
+ protected void testFencingOldClient(String oldClientVersion, String fencingVersion) {
+ String zookeeper = BookKeeperClusterUtils.zookeeperConnectString(docker)
+
+ def oldCL = MavenClassLoader.forBookKeeperVersion(oldClientVersion)
+ def oldBK = oldCL.newBookKeeper(zookeeper)
+ def fencingCL = MavenClassLoader.forBookKeeperVersion(fencingVersion)
+ def fencingBK = fencingCL.newBookKeeper(zookeeper)
+
+ try {
+ def numEntries = 5
+ def ledger0 = oldBK.createLedger(3, 2,
+ oldCL.digestType("CRC32"),
+ PASSWD)
+ for (int i = 0; i < numEntries; i++) {
+ ledger0.addEntry(("foobar" + i).getBytes())
+ }
+ ledger0.close()
+
+
+ def ledger1 = fencingBK.openLedger(ledger0.getId(), fencingCL.digestType("CRC32"), PASSWD)
+
+ // cannot write any more
+ try {
+ ledger0.addEntry("shouldn't work".getBytes())
+ Assert.fail("Shouldn't have been able to add any more")
+ } catch (Exception e) {
Review comment:
because we don't have it in the classpath at compile time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#issuecomment-999537020
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#issuecomment-999434442
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#issuecomment-999701224
@eolivelli Approved for what ever it's worth. Please feel free to commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#discussion_r773378954
##########
File path: tests/docker-images/statestore-image/image_builder.sh
##########
@@ -18,22 +18,34 @@
# * limitations under the License.
# */
+IMAGE_NAME=apachebookkeeper/bookkeeper-current:latest
+if [[ "$(docker images -q $IMAGE_NAME 2> /dev/null)" != "" ]]; then
+ echo "reusing local image: $IMAGE_NAME"
+ exit 0
Review comment:
One problem I see with this optimization is that, when debugging or locally running test, I would not know for sometime why changes I made is not taking affect, until I see that image_builder is not building new image anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#issuecomment-999030858
LGTM. Provided couple of observations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#issuecomment-1000117293
Great work @nicoloboschi
committed to master branch !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#discussion_r773378763
##########
File path: tests/docker-images/statestore-image/image_builder.sh
##########
@@ -18,22 +18,34 @@
# * limitations under the License.
# */
+IMAGE_NAME=apachebookkeeper/bookkeeper-current:latest
Review comment:
One problem I see with this optimization is that, when debugging or locally running test, I would not know for sometime why changes I made is not taking affect, until I see that image_builder is not building new image anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#issuecomment-1000117132
Prashant, IIUC you are a committer now, so your Approval is binding to commit the patch.
I wonder why you still haven't write permissions to the repository.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#discussion_r773709000
##########
File path: .github/workflows/backward-compat-tests.yml
##########
@@ -0,0 +1,58 @@
+#
Review comment:
good observation. I checked it and they run only in this job, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2937: Gradle - Enable backward-compatibility tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2937:
URL: https://github.com/apache/bookkeeper/pull/2937#discussion_r773385402
##########
File path: .github/workflows/backward-compat-tests.yml
##########
@@ -0,0 +1,58 @@
+#
Review comment:
Kindly also see to it that these tests are not running in the catch all tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org