-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Java 21 pipeline #11672
Java 21 pipeline #11672
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11672 +/- ##
============================================
- Coverage 66.48% 66.47% -0.02%
Complexity 207 207
============================================
Files 2343 2343
Lines 126418 126422 +4
Branches 19440 19443 +3
============================================
- Hits 84054 84035 -19
- Misses 36499 36519 +20
- Partials 5865 5868 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 24 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
7a1f476
to
38eb806
Compare
@@ -36,9 +36,7 @@ | |||
<properties> | |||
<pinot.root>${basedir}/../../..</pinot.root> | |||
<phase.prop>package</phase.prop> | |||
<scala.major.version>2.12</scala.major.version> |
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 module is still called spark pinot-batch-ingestion-spark-3.2
:p
I think we can either change it to pinot-batch-ingestion-spark-3
?
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.
Yep. I discussed about that in #11701 and its related issue. I'm ok with changing that name
here are too many CI checks, can we try to reduce the number of tests? |
Sure, we need to discuss which ones we want to have. My recommendation right now is:
Java 11 with skipbuffers true should also be enable, but given it doesn't pass the tests right now, we need to skip it. What do you think? |
I think we can just test the default byteBuffer usage. |
But we can skip it if you think it is not necessary |
I've changed the pipeline to run with:
So we have the same number of executions as before, but Java 17 and 20 have been replaced with Java 21 with and without bytebuffers. What do you think? |
c9e2ada
to
d63af27
Compare
Make sense, let's enable this for now and we can disable the tests once we have enough confidence. |
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/PinotDataBuffer.java
Outdated
Show resolved
Hide resolved
Seems there is a missing rebase for: #11804 |
Also one thing missing there is to add more codecov flags for skip buffer |
ff23549
to
6b3b853
Compare
The reason to be of this PR is to be able to progress on #11656.
Specifically it: