Skip to content

Conversation

@yew1eb
Copy link
Contributor

@yew1eb yew1eb commented Jan 23, 2026

Which issue does this PR close?

Closes #<issue_number>

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

How was this patch tested?

@github-actions github-actions bot added the infra label Jan 23, 2026
@yew1eb yew1eb force-pushed the improve_celeborn_ci branch from 5a95f1b to b20fb38 Compare January 23, 2026 14:24
@yew1eb yew1eb marked this pull request as draft January 23, 2026 15:04
@yew1eb yew1eb force-pushed the improve_celeborn_ci branch 2 times, most recently from 3473ae9 to d2f7c7d Compare January 23, 2026 15:55
@yew1eb yew1eb force-pushed the improve_celeborn_ci branch from d2f7c7d to bc0334e Compare January 23, 2026 16:07
@yew1eb yew1eb changed the title improve celeborn ci [AURON #1952] Add sort writer test for Celeborn integration CI Jan 23, 2026
@yew1eb yew1eb marked this pull request as ready for review January 23, 2026 16:07
@yew1eb
Copy link
Contributor Author

yew1eb commented Jan 23, 2026

depends on #1932

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds testing for the "sort" shuffle writer in addition to the existing "hash" writer for Celeborn integration CI. The PR expands the test matrix to cover both writer types across both Celeborn versions (0.5 and 0.6).

Changes:

  • Expanded the Celeborn CI matrix from 2 jobs to 4 jobs (2 Celeborn versions × 2 writer types)
  • Added overwrite: true parameter to artifact upload actions to handle artifact naming collisions when multiple matrix combinations reuse the same build artifacts
  • Updated job naming to include the writer parameter for better visibility

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
.github/workflows/celeborn.yml Expanded matrix to test both "hash" and "sort" shuffle writers for each Celeborn version, parametrized the writer configuration
.github/workflows/tpcds-reusable.yml Added overwrite: true to artifact uploads to handle multiple workflow calls with identical artifact names
Comments suppressed due to low confidence (1)

.github/workflows/tpcds-reusable.yml:177

  • The overwrite: true parameter is missing for this upload-artifact action. This is inconsistent with the other upload-artifact actions in this job (lines 171, 186, and 195) which all have overwrite: true set. Since the celeborn workflow now runs with a matrix that includes multiple writer configurations for the same celeborn version, this could lead to artifact name collisions. While the unit-tests-log upload only happens on failure, it's better to be consistent with the other uploads.
      - name: Upload unit tests log
        if: failure()
        uses: actions/upload-artifact@v6
        with:
          name: unit-tests-logs-${{ inputs.sparkver }}_${{ inputs.scalaver }}-jdk-${{ inputs.javaver }}${{ inputs.celebornver && format('-{0}', inputs.celebornver) || '' }}${{ inputs.unifflever && format('-{0}', inputs.unifflever) || '' }}
          path: "**/target/unit-tests.log"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant