-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
Upgrade oracle test version #24505
base: master
Are you sure you want to change the base?
Upgrade oracle test version #24505
Conversation
95c9ddd
to
578a6c5
Compare
Oracle test failed:
|
578a6c5
to
1a90bd2
Compare
plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestingOracleServer.java
Show resolved
Hide resolved
d4b697e
to
2051b06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comment
4cdd6b5
to
db92378
Compare
|
db92378
to
3e91b15
Compare
Please run stress tests by removing unnecessary jobs and duplicating Oracle job in |
1088b34
to
fbfc083
Compare
|
8d0d4ec
to
65a9f1c
Compare
053d648
to
1a4cd8a
Compare
Could you rebase on master to resolve conflicts? |
1a4cd8a
to
3dab010
Compare
7543512
to
2d3f891
Compare
@ebyhr @hashhar , I’d like your suggestions on how to minimize the logs during testing. To clarify, since I run multiple Oracle tests, only one test is expected to succeed, while the others encounter Currently, I’m using a Linux pipe to filter logs and only capture ERROR messages. This approach works and can report the correct error message for debug. But, I’m wondering if there’s a more natural or elegant way to handle this. |
.github/workflows/ci.yml
Outdated
run: | | ||
if [ "${{ matrix.modules }}" == "plugin/trino-oracle" ]; then | ||
echo "Running tests for oracle..." | ||
$MAVEN test ${MAVEN_TEST} -pl ${{ matrix.modules }} ${{ matrix.profile != '' && format('-P {0}', matrix.profile) || '' }} 2>&1 | grep "ERROR" || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, Here is the logic to filter only ERROR logs
72482f2
to
fa40ac8
Compare
This pull request has gone a while without any activity. Tagging for triage help: @mosabua |
If 18 is the highest version we test with .. we have to update the requirements section in the docs to 18 .. |
@mosabua agreed |
(rebased & solved conflict) |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chenjian2664 Is this PR ready for merge? How was the stress test result?
.github/workflows/ci.yml
Outdated
else | ||
echo "Running tests for ${{ matrix.modules }}..." | ||
$MAVEN test ${MAVEN_TEST} -pl ${{ matrix.modules }} ${{ matrix.profile != '' && format('-P {0}', matrix.profile) || '' }} | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we revert this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ebyhr Let me do more round tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ebyhr this change is for reduce the logs of the oracle tests, which is required of this pr.
Could you please see the stress test results? I just triggle tests of oracle, there is one successful and others failed at uploading the test result phrase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is for reduce the logs of the oracle tests, which is required of this pr.
Oh, I thought it's just for stress tests. Is it difficult to reduce logs within Oralce tests instead of ci.yml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which loggers are chatty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take 2 log in current oracle(version 11) logs, it's suspicious:
2025-02-11T21:35:00.494-0600 DEBUG SplitRunner-20250212_033459_00205_dxdmg.1.1.0-2-4910 oracle.jdbc {0} bytes written to the Socket. parameters=[8192]
Its frequency2025-02-11T21:35:00.499-0600 INFO SplitRunner-20250212_033459_00205_dxdmg.1.1.0-2-4910 oracle.jdbc properties={0}. parameters=[{LOCALE=en_US, DriverVersion=23.3.0.23.09, java.library.path: =/usr/java/packages/lib:/usr/lib64:/lib64:/lib:/usr/lib, java.class.path: =/home/runner/work/trino/trino/plugin/trino-oracle/target/test-classes:/home/runner/work/trino/trino/plugin/trino-oracle/target/classes:/home/runner/.m2/repository/com/google/guava/guava/33.4.0-jre/guava-33.4.0-jre.jar:/home/runner/.m2/repository/com/google/guava/failureaccess/1.0.2/failureaccess-1.0.2.jar:/home/runner/.m2/repository/com/google/code/findbugs/jsr305/3.0.2/jsr305-3.0.2.jar:/home/runner/.m2/repository/org/checkerframework/checker-qual/3.49.0/checker-qual-3.49.0.jar:/home/runner/.m2/repository/com/google/j2objc/j2objc-annotations/3.0.0/j2objc-annotations-3.0.0.jar:/home/runner/.m2/repository/com/google/inject/guice/7.0.0/guice-7.0.0.jar:/home/runner/.m2/repository/jakarta/inject/jakarta.inject-api/2.0.1/jakarta.inject-api-2.0.1.jar:/home/runner/.m2/repository/aopalliance/aopalliance/...
It's not that frequency like first one, but it's body more than 30k characters.
18 image is much larger than 11, thus is there is no enough remain space for logging so much content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wendigo Just in case you wanna see the failure about No space left
, may run multi oracle tests together, like stress test do, single run may not hit the failure sometimes.
ad9681d
to
cf16f4e
Compare
f3076ea
to
55a8dce
Compare
I see there still lots of logs like |
Description
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: