Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
b25a911
First attempt to add support for Java 17 and 20, including using Java…
gortiz Mar 27, 2023
619c2ae
Selectively disable checkstyle for Unsafe import in two classes
gortiz Apr 4, 2023
a2a10a2
Remove unused imports
gortiz Apr 4, 2023
4ba9038
Disable Unsafe import with suppressions instead of comments.
gortiz Apr 4, 2023
4dd8c04
Add comments on special configs
gortiz Apr 4, 2023
f01e7a0
Tests java 8 with adopt and 11, 15, 17 and 20 with temurin
gortiz Apr 4, 2023
ac15382
Temporally don't prioritize byte buffers
gortiz Apr 4, 2023
f3ed739
Run quickstarts of java 8 and 15 with adopt
gortiz Apr 4, 2023
dc22718
Remove pinot-foreign-memory from compilation unit
gortiz May 5, 2023
d902046
Force a more modern lombok version compatible with Java 16
gortiz May 5, 2023
2ee7460
Initialize direct memory as 0
gortiz May 5, 2023
db087e7
export sun packages in presto driver
gortiz May 5, 2023
80a28c6
Fix assert that only worked in Java <14
gortiz May 5, 2023
19e1508
Support map of 0 length in MmapMemory
gortiz May 5, 2023
910ffbd
Remove deprecated TODO
gortiz May 5, 2023
a7d4dd1
Skip LArray tests in Java > 15, as it is not supported
gortiz May 5, 2023
9352af9
Updated mockito version to work in Java 17
gortiz May 5, 2023
90a5d0f
Fix UnsafePinotBuffer.toDirectByteBuffer not honoring requested byte …
gortiz May 5, 2023
f535368
Fix VarByteChunkSVForwardIndexTest explicitly asking for LArray buffers
gortiz May 5, 2023
d211770
Add --java-opens as JVM_OPT in pinot_tool script when Java > 11
gortiz May 5, 2023
d264fcd
Remove $PINOT_JAVA_VERSION variable in GH tests to use already existi…
gortiz May 5, 2023
7d477ff
Use UnsafePinotBufferFactory by default
gortiz May 8, 2023
28f1660
add some opens required to use jersey 2.35, kinesis and flink
gortiz May 8, 2023
50eb9ae
Temporally remove presto compilation with Java 11.
gortiz May 8, 2023
44ae726
Use LArray on java < 16
gortiz May 16, 2023
338b96c
Changes on BenchmarkPinotDataBuffer
gortiz May 16, 2023
1d97eba
revert changes on pom.xml. --add-opens need to be included in surefire
gortiz May 16, 2023
aa3560e
fix checkstyle
gortiz May 16, 2023
22121e7
More changes in BenchmarkPinotDataBuffer.java
gortiz May 16, 2023
aa9bdc7
Add support for Java 21
gortiz May 16, 2023
abe9ac5
Enable params 1 and 32 and wrapper+unsafe
gortiz May 16, 2023
3217bb6
Temporally do not cancel tests in GA when one of them fail
gortiz May 17, 2023
e76fb47
Use Java 21-ea in tests
gortiz May 17, 2023
f4f6224
Use Unsafe by default in all cases to test the code
gortiz May 17, 2023
967114e
Remove Java 21 from GA, as it is not supported
gortiz May 17, 2023
6d0b502
Run tests with Java 20
gortiz May 18, 2023
ba0f427
prioritize byte buffers by default
gortiz May 19, 2023
576cf0f
Add a PinotDataBuffer.createDefaultFactory(boolean) method to configu…
gortiz May 19, 2023
4169ea3
remove add-export java.base/jdk.internal.util.random
gortiz May 22, 2023
81f8371
Remove java 20 from tests, as some mvn plugins don't work on java 20
gortiz May 22, 2023
f4a55d1
Re-add export jdk.internal.util.random
gortiz May 22, 2023
53f2a95
Move --add-open used as test jvm args to properties
gortiz May 22, 2023
8c0d334
Set -add-open in github-actions profile
gortiz May 22, 2023
b05691b
Change pom.argLine property
gortiz May 23, 2023
4b3476d
Remove foreign memory wrapper source code. Maven project was already …
gortiz May 23, 2023
aa303bb
Remove commented code
gortiz May 23, 2023
b739b00
Remove incorrect comment
gortiz May 23, 2023
b02702d
Improve error logging
gortiz May 24, 2023
76d5ea6
Trying to find the reason why MultiNodesOfflineClusterIntegrationTest…
gortiz May 24, 2023
b8e631d
testServerHardFailure should always restart stopped server
gortiz May 24, 2023
16fb3b0
Fix testServerHardFailure
gortiz May 24, 2023
8e97581
Remove deprecated comments in PinotDefaultByteBufferTest
gortiz May 24, 2023
e575174
Remove unused imports
gortiz May 24, 2023
9ab14ca
Add a tentative implementation using Chronicle Core and Chronicle Posix.
gortiz May 25, 2023
bd4720f
Make dependencies converge
gortiz May 25, 2023
90485ee
Add openhft dependencies in presto subprojects
gortiz May 26, 2023
cdb615c
GitHub jobs that run tests will not verify checkstyle, license or spo…
gortiz May 29, 2023
de96879
Include actual Java version used in tests jobs
gortiz May 29, 2023
27a6905
Remove a double -Dcheckstyle.skip in integration-test 2
gortiz May 29, 2023
ebcfc26
nl.jqno.equalsverifier:equalsverifier:3.4 doesn’t work with Java 20. …
gortiz May 29, 2023
c073a1f
run unit tests with Java 20
gortiz May 29, 2023
41002cc
Change -DskipTests with -Dmaven.test.skip=true in some cases.
gortiz May 29, 2023
94d4f89
Change a test that failed once mockito was updated.
gortiz May 29, 2023
44048e5
Remove quickstart 8 from GA tests
gortiz May 30, 2023
b2e96b3
Honor pinot.offheap.prioritize.bytebuffer config when pinot.offheap.b…
gortiz May 31, 2023
8f4b76b
Remove a TODO comment
gortiz May 31, 2023
f718193
Add finalize in both buffers and memory
gortiz Jun 1, 2023
d0e936b
Improved javadoc
gortiz Jun 1, 2023
3cb4e1c
Fix a possible problem when flushing mmaps in Java 20
gortiz Jun 1, 2023
6bb99a1
Remove finalize from PinotDataBuffer
gortiz Jun 1, 2023
e95b0b7
Instantiate PinotBufferFactory with PluginManager
gortiz Jun 6, 2023
55d1f87
Load the PinotDataFactory using PinotConfiguration
gortiz Jun 6, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 26 additions & 34 deletions .github/workflows/pinot_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,20 @@ jobs:
if: github.repository == 'apache/pinot'
runs-on: ubuntu-latest
strategy:
# Changed to false in order to improve coverage using unsafe buffers
fail-fast: false
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can change this and other introduced fail-fast: false once we are happy with the code

matrix:
testset: [ 1, 2 ]
name: Pinot Unit Test Set ${{ matrix.testset }}
java: [ 11, 17, 20 ]
distribution: [ "temurin" ]
name: Pinot Unit Test Set ${{ matrix.testset }} (${{matrix.distribution}}-${{matrix.java}})
steps:
- uses: actions/checkout@v3
- name: Set up JDK 11
- name: Set up JDK ${{ matrix.java }}
uses: actions/setup-java@v3
with:
java-version: 11
distribution: 'adopt'
java-version: ${{ matrix.java }}
distribution: ${{ matrix.distribution }}
cache: 'maven'
# Step that does that actual cache save and restore
- uses: actions/cache@v3
Expand Down Expand Up @@ -111,7 +115,7 @@ jobs:
continue-on-error: true
timeout-minutes: 5
with:
flags: unittests${{ matrix.testset }}
flags: unittests${{ matrix.testset }}${{matrix.distribution}}${{matrix.java}}
name: codecov-unit-tests
fail_ci_if_error: false
verbose: true
Expand All @@ -120,16 +124,20 @@ jobs:
if: github.repository == 'apache/pinot'
runs-on: ubuntu-latest
strategy:
# Changed to false in order to improve coverage using unsafe buffers
fail-fast: false
matrix:
testset: [ 1, 2 ]
name: Pinot Integration Test Set ${{ matrix.testset }}
java: [ 11, 17, 20 ]
distribution: [ "temurin" ]
name: Pinot Integration Test Set ${{ matrix.testset }} (${{matrix.distribution}}-${{matrix.java}})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

will the github actions end to end now take 6 times as longer than before? Or would they run in parallel? If the latter, do we know that we have enough resources on there to handle the increased num tasks?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will the github actions end to end now take 6 times as longer than before? Or would they run in parallel?

They will run in parallel. Tests on this branch finish in about 1 hour, as usual.

do we know that we have enough resources on there to handle the increased num tasks?

I don't know the resources we have. We are going to consume more resources for sure.

steps:
- uses: actions/checkout@v3
- name: Set up JDK 11
- name: Set up JDK ${{ matrix.java }}
uses: actions/setup-java@v3
with:
java-version: 11
distribution: 'adopt'
java-version: ${{ matrix.java }}
distribution: ${{ matrix.distribution }}
cache: 'maven'
# Step that does that actual cache save and restore
- uses: actions/cache@v3
Expand Down Expand Up @@ -159,7 +167,7 @@ jobs:
continue-on-error: true
timeout-minutes: 5
with:
flags: integration${{ matrix.testset }}
flags: integration${{ matrix.testset }}${{matrix.distribution}}${{matrix.java}}
name: codecov-integration-tests
fail_ci_if_error: false
verbose: true
Expand All @@ -168,6 +176,8 @@ jobs:
if: github.repository == 'apache/pinot'
runs-on: ubuntu-latest
strategy:
# Changed to false in order to improve coverage using unsafe buffers
fail-fast: false
matrix:
test_suite: [ "compatibility-verifier/sample-test-suite" ]
old_commit: [
Expand Down Expand Up @@ -221,18 +231,19 @@ jobs:
if: github.repository == 'apache/pinot'
runs-on: ubuntu-latest
strategy:
# Changed to false in order to improve coverage using unsafe buffers
fail-fast: false
matrix:
# We only test LTS Java versions
# TODO: add JDK 17 once ready. see: https://github.com/apache/pinot/issues/8529
java: [ 8, 11, 15 ]
java: [ 11, 17, 20 ]
Comment thread
gortiz marked this conversation as resolved.
Outdated
distribution: [ "temurin" ]
name: Pinot Quickstart on JDK ${{ matrix.java }}
steps:
- uses: actions/checkout@v3
- name: Set up JDK ${{ matrix.java }}
uses: actions/setup-java@v3
with:
java-version: ${{ matrix.java }}
distribution: 'adopt'
distribution: ${{ matrix.distribution }}
cache: 'maven'
# Step that does that actual cache save and restore
- uses: actions/cache@v3
Expand All @@ -252,25 +263,6 @@ jobs:
name: Build Presto Pinot Driver
steps:
- uses: actions/checkout@v3
- name: Set up JDK 11
uses: actions/setup-java@v3
with:
java-version: 11
distribution: 'adopt'
cache: 'maven'
- name: Build presto pinot driver with JDK 11
env:
MAVEN_OPTS: >
-Xmx2G -DfailIfNoTests=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=25
-Dmaven.wagon.http.retryHandler.count=30 -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false
-XX:+IgnoreUnrecognizedVMOptions
--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED
run: |
mvn clean install -DskipTests -Ppresto-driver -am -B -pl ':presto-pinot-driver' -T 16 || exit 1
- name: Set up JDK 8
uses: actions/setup-java@v3
with:
Expand All @@ -281,4 +273,4 @@ jobs:
env:
MAVEN_OPTS: -Xmx2G -DfailIfNoTests=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=25 -Dmaven.wagon.http.retryHandler.count=30 -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false
run: |
mvn clean install -DskipTests -Ppresto-driver -am -B -pl ':presto-pinot-driver' -Djdk.version=8 -T 16 || exit 1
mvn clean install -Dmaven.test.skip=true -Ppresto-driver -am -B -pl ':presto-pinot-driver' -Djdk.version=8 -T 16 || exit 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this change required? Or both ways work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

maven.test.skip implies skipTests. The main difference is that maven.test.skip does not compile and does not run the tests, while skipTests compiles them but do not run them. Apart from getting some cycles, the main reason to skip the compilation is to avoid errors there. I've found two different issues:

  • Some plugins do not run with modern jvms
  • Some libraries we use have been compiled with java 11
    The latter is the the reason why we need to skip compilation here. The environment is here configured to run with Java 8 in order to compile presto, but introduced a library that uses Java 11 (IIRC the library is mockito). We updated these libraries in order to be able to run in Java 20 (in order to have it ready for Java 21).

5 changes: 3 additions & 2 deletions .github/workflows/scripts/.pinot_quickstart.sh
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ PASS=0
for i in $(seq 1 2)
do
if [ "$JAVA_VER" -gt 11 ] ; then
mvn clean install -B -DskipTests=true -Pbin-dist -Dmaven.javadoc.skip=true -Djdk.version=11
mvn clean install -B -Dmaven.test.skip=true -Pbin-dist -Dmaven.javadoc.skip=true -Djdk.version=11
else
mvn clean install -B -DskipTests=true -Pbin-dist -Dmaven.javadoc.skip=true -Djdk.version=${JAVA_VER}
mvn clean install -B -Dmaven.test.skip=true -Pbin-dist -Dmaven.javadoc.skip=true -Djdk.version=${JAVA_VER}
fi
if [ $? -eq 0 ]; then
PASS=1
Expand All @@ -87,6 +87,7 @@ cd "${DIST_BIN_DIR}"

# Test standalone pinot. Configure JAVA_OPTS for smaller memory, and don't use System.exit
export JAVA_OPTS="-Xms1G -Dlog4j2.configurationFile=conf/log4j2.xml"

bin/pinot-admin.sh StartZookeeper &
ZK_PID=$!
sleep 10
Expand Down
18 changes: 12 additions & 6 deletions .github/workflows/scripts/.pinot_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,21 @@ netstat -i

if [ "$RUN_INTEGRATION_TESTS" != false ]; then
# Integration Tests
mvn clean install -DskipTests -am -B -pl 'pinot-integration-tests' -T 16 || exit 1
mvn clean install -DskipTests -Dcheckstyle.skip -Dspotless.apply.skip -Dlicense.skip=true -am -B \
-pl 'pinot-integration-tests' -T 16 || exit 1
if [ "$RUN_TEST_SET" == "1" ]; then
mvn test -am -B \
-pl 'pinot-integration-tests' \
-Dtest='C*Test,L*Test,M*Test,R*Test,S*Test' \
-P github-actions,integration-tests-only && exit 0 || exit 1
-P github-actions,integration-tests-only \
-Dcheckstyle.skip -Dspotless.apply.skip -Dlicense.skip=true && exit 0 || exit 1
fi
if [ "$RUN_TEST_SET" == "2" ]; then
mvn test -am -B \
-pl 'pinot-integration-tests' \
-Dtest='!C*Test,!L*Test,!M*Test,!R*Test,!S*Test' \
-P github-actions,integration-tests-only && exit 0 || exit 1
-P github-actions,integration-tests-only \
-Dcheckstyle.skip -Dspotless.apply.skip -Dlicense.skip=true && exit 0 || exit 1
fi
else
# Unit Tests
Expand All @@ -60,10 +63,11 @@ else
-pl ':pinot-csv' \
-pl ':pinot-json' \
-pl ':pinot-segment-uploader-default' \
-P github-actions,no-integration-tests && exit 0 || exit 1
-P github-actions,no-integration-tests \
-Dcheckstyle.skip -Dspotless.apply.skip -Dlicense.skip=true && exit 0 || exit 1
fi
if [ "$RUN_TEST_SET" == "2" ]; then
mvn clean install -DskipTests -T 16 || exit 1
mvn clean install -DskipTests -Dcheckstyle.skip -Dspotless.apply.skip -Dlicense.skip=true -T 16 || exit 1
mvn test -am -B \
-pl '!pinot-spi' \
-pl '!pinot-segment-spi' \
Expand All @@ -79,7 +83,9 @@ else
-pl '!:pinot-csv' \
-pl '!:pinot-json' \
-pl '!:pinot-segment-uploader-default' \
-P github-actions,no-integration-tests && exit 0 || exit 1
-P github-actions,no-integration-tests \
-Dspotless.apply.skip -Dcheckstyle.skip -Dspotless.apply.skip -Dlicense.skip=true \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why add skips for all these?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are 3 reasons:

  1. It is not actually needed. These are already verified by the linter GA job, so we are not going to miss them.
  2. Subjectively, it is problematic. With this changes, tests are going to be executed even when something with linter errors is pushed. That means that only linter fails (ie I usually forget to add the headers) but tests pass, you can be confident than once the linter is fixed, everything should go green.
  3. Objectively, there were problems when not skipping them. Mainly, spotless doesn't work right now in Java 20. Therefore the change was necessary to run the tests.

&& exit 0 || exit 1
fi
fi

Expand Down
3 changes: 3 additions & 0 deletions config/suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,7 @@

<!-- Allow static imports in tests -->
<suppress checks="AvoidStaticImport" files="[\\/]src[\\/]test[\\/].*" />

<suppress checks="IllegalImport" files="Unsafer.java"/>
<suppress checks="IllegalImport" files="UnsafePinotBuffer.java"/>
Comment thread
gortiz marked this conversation as resolved.
Outdated
</suppressions>
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import org.apache.pinot.segment.spi.utils.JavaVersion;
import org.testng.annotations.Test;

import static org.testng.Assert.*;
Expand Down Expand Up @@ -105,7 +106,10 @@ public void testToIterable() {
} catch (NullPointerException e) {
// It's supposed to get a JsonPathException, but JsonPath library actually
// has a bug leading to NullPointerException while creating the JsonPathException.
assertNull(e.getMessage());
if (JavaVersion.VERSION < 14) {
// In modern Java versions messages is something like "Cannot invoke "Object.getClass()" because "obj" is null"
assertNull(e.getMessage());
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@
<groupId>org.xerial.larray</groupId>
<artifactId>larray-mmap</artifactId>
</dependency>
<dependency>
<groupId>net.openhft</groupId>
<artifactId>posix</artifactId>
</dependency>
<dependency>
<groupId>net.openhft</groupId>
<artifactId>chronicle-core</artifactId>
</dependency>

<!-- test -->
<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import org.apache.pinot.spi.utils.CommonConstants.Broker.FailureDetector;
import org.apache.pinot.spi.utils.CommonConstants.Helix.StateModel.BrokerResourceStateModel;
import org.apache.pinot.util.TestUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testng.annotations.Test;

import static org.testng.Assert.assertEquals;
Expand All @@ -45,6 +47,7 @@
* Integration test that extends OfflineClusterIntegrationTest but start multiple brokers and servers.
*/
public class MultiNodesOfflineClusterIntegrationTest extends OfflineClusterIntegrationTest {
private static final Logger LOGGER = LoggerFactory.getLogger(MultiNodesOfflineClusterIntegrationTest.class);
private static final int NUM_BROKERS = 2;
private static final int NUM_SERVERS = 3;

Expand Down Expand Up @@ -138,6 +141,7 @@ public void testServerHardFailure()
testCountStarQuery(3, false);
assertEquals(getCurrentCountStarResult(), expectedCountStarResult);

LOGGER.warn("Shutting down server " + _serverStarters.get(NUM_SERVERS - 1).getInstanceId());
// Take a server and shut down its query server to mimic a hard failure
BaseServerStarter serverStarter = _serverStarters.get(NUM_SERVERS - 1);
try {
Expand Down Expand Up @@ -174,7 +178,11 @@ private void testCountStarQuery(int expectedNumServersQueried, boolean exception
assertEquals(exceptions.size(), 2);
JsonNode firstException = exceptions.get(0);
assertEquals(firstException.get("errorCode").intValue(), QueryException.BROKER_REQUEST_SEND_ERROR_CODE);
assertTrue(firstException.get("message").textValue().contains("Connection refused"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel we should't remove this check. Are you removing it in order to pass the test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The check is incorrect. I think I already wrote about that somewhere, but can I do it here.

There is a race condition between the server that is being stopped and the requests that arrive. Normally, the shutdown is executed before, so the error that the later request receives is the expected Connection refused. But there are cases where the request arrives before the shutdown is executed and therefore the broker kills the connection once it was established, producing a Connection reset message. It could even be possible to receive a success if the request is processed before the server is stopped.

The correct solution would be to break the race condition by sequentializing the calls. In order to do that the stop request should be blocking. I don't know how to do that and it may introduce some extra complexity on the PR, so I thought it would be better to just do not check the message. We are already checking the error code, which should be good enough.

Also, checking error messages is something problematic in general. It may depend, for example, on the locale configured by the used, which would make the tests difficult to reproduce. Instead we should include more specific error codes.

String firstExceptionMessage = firstException.get("message").textValue();
if (!firstExceptionMessage.contains("Connection refused")) {
LOGGER.warn("first exception message is " + firstExceptionMessage + ", which does not contain "
+ "\"Connection refused\"");
}
JsonNode secondException = exceptions.get(1);
assertEquals(secondException.get("errorCode").intValue(), QueryException.SERVER_NOT_RESPONDING_ERROR_CODE);
} else {
Expand Down
Loading