Revised IT to detect backward incompatible change #16779
Conversation
| DRUID_PREVIOUS_VERSION_DOWNLOAD_URL: | ||
| required: false | ||
| type: string | ||
| DRUID_PREVIOUS_IT_IMAGE_NAME: |
There was a problem hiding this comment.
I think we can remove this image name. We can easily derive it from the DRUID_PREVIOUS_VERSION
There was a problem hiding this comment.
Done, added a job to derive it internally.
| @@ -57,6 +57,19 @@ on: | |||
| AWS_SECRET_ACCESS_KEY: | |||
| required: false | |||
| type: string | |||
| BACKWARD_INCOMPATIBILITY_IT_ENABLED: | |||
There was a problem hiding this comment.
I think we can remove this as well. If druid previous version is null, we can just skip running the backward compatibility it.
|
|
||
| package org.apache.druid.testsEx.categories; | ||
|
|
||
| public class BackwardIncompatibility |
There was a problem hiding this comment.
| public class BackwardIncompatibility | |
| public class BackwardCompatibility |
Nit
|
|
||
| ENV DRUID_HOME=/usr/local/druid | ||
|
|
||
| # Populate build artifacts | ||
|
|
||
| COPY apache-druid-${DRUID_VERSION}-bin.tar.gz /usr/local/ | ||
| COPY druid-it-tools-${DRUID_VERSION}.jar /tmp/druid/extensions/druid-it-tools/ | ||
| COPY druid-it-tools-${DRUID_TESTING_TOOLS_VERSION}.jar /tmp/druid/extensions/druid-it-tools/ |
There was a problem hiding this comment.
Can we make loading this testing tool version optional ?
There was a problem hiding this comment.
This doesn't seem trivial.
The old image wouldn't run since the extension is added in the common extension list here https://github.com/apache/druid/blob/master/integration-tests-ex/cases/cluster/Common/environment-configs/common.env#L53.
The IT framework allows extending the config file to add more extensions but there is no way to remove an extension.
There was a problem hiding this comment.
Adding a note on this limitation in description and test readme file.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (sqlTaskStatus.getState().isFailure()) { | ||
| Assert.fail(StringUtils.format( | ||
| "Unable to start the task successfully.\nPossible exception: %s", | ||
| sqlTaskStatus.getError() |
Check notice
Code scanning / CodeQL
Use of default toString() Note test
| if (sqlTaskStatus.getState().isFailure()) { | ||
| Assert.fail(StringUtils.format( | ||
| "Unable to start the task successfully.\nPossible exception: %s", | ||
| sqlTaskStatus.getError() |
Check notice
Code scanning / CodeQL
Use of default toString() Note test
| if (exportTask.getState().isFailure()) { | ||
| Assert.fail(StringUtils.format( | ||
| "Unable to start the task successfully.\nPossible exception: %s", | ||
| exportTask.getError() |
Check notice
Code scanning / CodeQL
Use of default toString() Note test
| if (sqlTaskStatus.getState().isFailure()) { | ||
| Assert.fail(StringUtils.format( | ||
| "Unable to start the task successfully.\nPossible exception: %s", | ||
| sqlTaskStatus.getError() |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| if (sqlTaskStatus.getState().isFailure()) { | ||
| Assert.fail(StringUtils.format( | ||
| "Unable to start the task successfully.\nPossible exception: %s", | ||
| sqlTaskStatus.getError() |
Check notice
Code scanning / CodeQL
Use of default toString()
| if (exportTask.getState().isFailure()) { | ||
| Assert.fail(StringUtils.format( | ||
| "Unable to start the task successfully.\nPossible exception: %s", | ||
| exportTask.getError() |
Check notice
Code scanning / CodeQL
Use of default toString()
|
All the revised ITs have run successfully (cc31175). Disabling this IT in Apache for now. |
|
|
||
| package org.apache.druid.testsEx.categories; | ||
|
|
||
| public class BackwardCompatibilityMain |
There was a problem hiding this comment.
Please add a small javadoc here, in particular pointing out what the "Main" stands for.
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public class MultiStageQuery |
There was a problem hiding this comment.
Please rename to MultiStageQueryTest.
|
|
||
| @RunWith(DruidTestRunner.class) | ||
| @Category(BackwardCompatibilityMain.class) | ||
| public class ITBCMainIndexerTest extends IndexerTest |
There was a problem hiding this comment.
Can we drop the BCMain in the name?
It is redundant since the class is already in the backward compat package.
Added a new revised IT group
BackwardCompatibilityMain. The idea is to catch potential backward compatibility issues that may arise during rolling upgrade.This test group runs a docker-compose cluster with Overlord & Coordinator service on the previous druid version.
Following env vars are required in the GHA file
.github/workflows/unit-and-integration-tests-unified.ymlto run this testNote, that this test group will remain disabled on Apache until we find a way to host druid tar in a separate location.
Limitations
druid-testing-toolsjar is not published. The image for the previous version still uses theextension from the current build. This could break in case of incompatible changes in this extension.
In such a scenario the test should be disabled. However, this extension is primarily used to
test specific error scenarios and launch custom node role service (used in HighAvailability test group).
This PR has: