You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "grundprinzip (via GitHub)" <gi...@apache.org> on 2023/03/23 13:03:45 UTC
[GitHub] [spark] grundprinzip opened a new pull request, #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
grundprinzip opened a new pull request, #39294:
URL: https://github.com/apache/spark/pull/39294
### What changes were proposed in this pull request?
This PR adds a new script to check for breaking changes in the Spark Connect protos using the `buf` breaking changes detector.
Please see here for more details: https://docs.buf.build/tour/detect-breaking-changes
### Why are the changes needed?
Testing / CI / CD
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Manual WF testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1368178332
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1367604768
Check working as expected:
<img width="1266" alt="Screenshot 2022-12-29 at 22 58 19" src="https://user-images.githubusercontent.com/3421/210014930-bdea97bf-d774-4d01-ae5d-f6c3d90ebda5.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on a diff in pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1144009897
##########
.github/workflows/build_and_test.yml:
##########
@@ -493,6 +494,80 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check-master:
+ needs: [precondition]
+ if: always() && fromJson(needs.precondition.outputs.required).proto-breaking-changes-check == 'true'
+ name: Spark Connect proto backwards compatibility check (master)
+ continue-on-error: true
Review Comment:
continue-on-error will let the workflow continue even if the job fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1367741801
buf can only validate the behavior on the protocol level but not on the side of the implementation.
If a newer client talks to an older server only the older surfaces is supported but it will not break.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1059728513
##########
.github/workflows/build_and_test.yml:
##########
@@ -494,6 +495,41 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check:
+ needs: [precondition]
+ if: always() && fromJson(needs.precondition.outputs.required).proto-breaking-changes-check == 'true'
+ name: Spark Connect Proto Backwards Compat Check
Review Comment:
nit but I would name it without capitalizing to be consistent with others. e.g.,
```suggestion
name: Check Protobuf backward compatibility
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1481159769
I think your original approaches without these plugins would be the quickest way ....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1367870466
That's the point of making it it's own branch in the workflow we can override it. But not doing it can cause issues even now.
I don't see a benefit in waiting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1368422894
> > If everything else is green and only the Buf check is read someone with the power to merge can decide to simply merge the change.
>
>
>
> If breaking change is allowed to merge, how should we record the incompatibility to inform the user when new version release? I understand that `MimaExcludes` is similar to an audit record.
That's a great point. Let me look into how we could track this.
I'm wondering if it's slightly different for Mima because the base version is pinned.
I think we will probably need a track record of breaking changes.
One thing I'm wondering is if we should relax the breaking changes check from FILE to WIRE to avoid that simple renames or deprecating fields breaks compatible.
Another thing is, let me add a readme about the proposed compatibility requirements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1497058436
Made a followup: https://github.com/apache/spark/pull/40673 PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1146148427
##########
.github/workflows/build_and_test.yml:
##########
@@ -493,6 +494,80 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check-master:
+ needs: [precondition]
+ if: always() && fromJson(needs.precondition.outputs.required).proto-breaking-changes-check == 'true'
+ name: Spark Connect proto backwards compatibility check (master)
+ continue-on-error: true
+ runs-on: ubuntu-22.04
+ env:
+ LC_ALL: C.UTF-8
+ LANG: C.UTF-8
+ PYSPARK_DRIVER_PYTHON: python3.9
+ PYSPARK_PYTHON: python3.9
+ steps:
+ - name: Checkout Spark repository
+ uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
+ repository: apache/spark
+ ref: ${{ inputs.branch }}
+ - name: Add GITHUB_WORKSPACE to git trust safe.directory
+ run: |
+ git config --global --add safe.directory ${GITHUB_WORKSPACE}
+ - name: Sync the current branch with the latest in Apache Spark
+ if: github.repository != 'apache/spark'
+ run: |
+ git fetch https://github.com/$GITHUB_REPOSITORY.git ${GITHUB_REF#refs/heads/}
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' merge --no-commit --progress --squash FETCH_HEAD
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' commit -m "Merged commit" --allow-empty
+ # Install the `buf` CLI
+ - uses: bufbuild/buf-setup-action@v1
+ with:
+ github_token: ${{ secrets.GITHUB_TOKEN }}
+ # Run buf breaking.
+ - uses: bufbuild/buf-breaking-action@v1
+ with:
+ input: connector/connect/common/src/main
+ against: 'https://github.com/apache/spark.git#branch=master,subdir=connector/connect/common/src/main'
Review Comment:
I still think we should make this warning instead of failing with a red box. Let me probably follow up and make a fix to make it look like a warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1481146691
Merged to master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1368422290
> If everything else is green and only the Buf check is read someone with the power to merge can decide to simply merge the change.
If breaking change is allowed to merge, how should we record the incompatibility to inform the user when new version release? I understand that `MimaExcludes` is similar to an audit record.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1367864372
@HyukjinKwon PTAL, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on a diff in pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1144010201
##########
.github/workflows/build_and_test.yml:
##########
@@ -493,6 +494,80 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check-master:
+ needs: [precondition]
+ if: always() && fromJson(needs.precondition.outputs.required).proto-breaking-changes-check == 'true'
+ name: Spark Connect proto backwards compatibility check (master)
+ continue-on-error: true
+ runs-on: ubuntu-22.04
+ env:
+ LC_ALL: C.UTF-8
+ LANG: C.UTF-8
+ PYSPARK_DRIVER_PYTHON: python3.9
+ PYSPARK_PYTHON: python3.9
+ steps:
+ - name: Checkout Spark repository
+ uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
+ repository: apache/spark
+ ref: ${{ inputs.branch }}
+ - name: Add GITHUB_WORKSPACE to git trust safe.directory
+ run: |
+ git config --global --add safe.directory ${GITHUB_WORKSPACE}
+ - name: Sync the current branch with the latest in Apache Spark
+ if: github.repository != 'apache/spark'
+ run: |
+ git fetch https://github.com/$GITHUB_REPOSITORY.git ${GITHUB_REF#refs/heads/}
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' merge --no-commit --progress --squash FETCH_HEAD
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' commit -m "Merged commit" --allow-empty
+ # Install the `buf` CLI
+ - uses: bufbuild/buf-setup-action@v1
+ with:
+ github_token: ${{ secrets.GITHUB_TOKEN }}
+ # Run buf breaking.
+ - uses: bufbuild/buf-breaking-action@v1
+ with:
+ input: connector/connect/common/src/main
+ against: 'https://github.com/apache/spark.git#branch=master,subdir=connector/connect/common/src/main'
+
+ proto-breaking-changes-check-release:
+ needs: [precondition]
+ if: always() && fromJson(needs.precondition.outputs.required).proto-breaking-changes-check == 'true'
+ name: Spark Connect proto backwards compatibility check (master)
+ runs-on: ubuntu-22.04
+ env:
+ LC_ALL: C.UTF-8
+ LANG: C.UTF-8
+ PYSPARK_DRIVER_PYTHON: python3.9
+ PYSPARK_PYTHON: python3.9
+ steps:
+ - name: Checkout Spark repository
+ uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
+ repository: apache/spark
+ ref: ${{ inputs.branch }}
+ - name: Add GITHUB_WORKSPACE to git trust safe.directory
+ run: |
+ git config --global --add safe.directory ${GITHUB_WORKSPACE}
+ - name: Sync the current branch with the latest in Apache Spark
+ if: github.repository != 'apache/spark'
+ run: |
+ git fetch https://github.com/$GITHUB_REPOSITORY.git ${GITHUB_REF#refs/heads/}
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' merge --no-commit --progress --squash FETCH_HEAD
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' commit -m "Merged commit" --allow-empty
+ # Install the `buf` CLI
+ - uses: bufbuild/buf-setup-action@v1
+ with:
+ github_token: ${{ secrets.GITHUB_TOKEN }}
+ # Run buf breaking.
+ - uses: bufbuild/buf-breaking-action@v1
+ with:
+ input: connector/connect/common/src/main
+ against: 'https://github.com/apache/spark.git#branch=branch-3.4,subdir=connector/connect/common/src/main'
Review Comment:
runs against the 3.4 release branch, this has to be updated on every release to the previous branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1478606975
Verifying that the broken build is reported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1478618853
> 
>
> Verifying that the broken build is reported.
cc @zhenlineo as I recall you thought about field renaming? Seems like that is not allowed per current design.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1367869447
Hmmm.. can we add this after 3.4 release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1368423420
There is another thing. We need the pre merge check to make sure we understand what's going on and potentially something like Mima excludes when comparing against the released branches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on a diff in pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1059648119
##########
.github/workflows/build_and_test.yml:
##########
@@ -494,6 +495,43 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check:
+ needs: [precondition, infra-image]
Review Comment:
The build for the check has passes without the `infra-image` closing the comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
URL: https://github.com/apache/spark/pull/39294
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1368399882
Had a offline discussion. I am fine with adding this check because:
- Detect the breaking changes (after the release)
- Let developers know what's breaking change (from now on).
- There aren't any major refactoring coming, and there's only two weeks left before the branch-cut date.
- intend to check these compatibility internally (in production)
So I am fine with adding this now but I don't mind holding off until the branchcut if anyone has a different thought. cc @dongjoon-hyun, @viirya, @mridulm, @Yikun, @squito, @tgravescs and @LuciferYang.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1367724555
After reading the `buf` documentation, `buf` seems to only allow file/directories exclusion check: https://docs.buf.build/breaking/configuration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a diff in pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
Yikun commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1059599071
##########
.github/workflows/build_and_test.yml:
##########
@@ -494,6 +495,43 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check:
+ needs: [precondition, infra-image]
+ if: always() && fromJson(needs.precondition.outputs.required).lint == 'true'
Review Comment:
s/lint/proto-breaking-changes-check/
##########
connector/connect/dev/detect_breaking_changes.sh:
##########
@@ -0,0 +1,36 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+if [[ $# -gt 1 ]]; then
+ echo "Illegal number of parameters."
+ echo "Usage: ./connector/connect/dev/generate_protos.sh [path]"
Review Comment:
detect_breaking_changes.sh...
##########
.github/workflows/build_and_test.yml:
##########
@@ -494,6 +495,43 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check:
+ needs: [precondition, infra-image]
Review Comment:
qq: Does the `detect` script depends on infra image (such python deps) or not?
I only saw `buf` in script and already installed in github action, so do we really need the infra-image?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1496981049
It's allowed back (https://issues.apache.org/jira/browse/INFRA-24404). Let me revert the revert and see if it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1496983890
Merged back in https://github.com/apache/spark/commit/0b45a5278026c2ea9ce2b127333514f7a7a933f4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
URL: https://github.com/apache/spark/pull/39294
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1481155253
uhoh ..
https://github.com/apache/spark/actions/runs/4500801526
Reverting for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on a diff in pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1059628577
##########
.github/workflows/build_and_test.yml:
##########
@@ -494,6 +495,43 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check:
+ needs: [precondition, infra-image]
+ if: always() && fromJson(needs.precondition.outputs.required).lint == 'true'
Review Comment:
Done.
##########
connector/connect/dev/detect_breaking_changes.sh:
##########
@@ -0,0 +1,36 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+if [[ $# -gt 1 ]]; then
+ echo "Illegal number of parameters."
+ echo "Usage: ./connector/connect/dev/generate_protos.sh [path]"
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1367732428
> For a breaking change which break the binary compatibility, we can add it to `MimaExcludes` after confirmation.
>
> How can we make braking changes in proto? Is it forbidden?
generally, yes. We must not make breaking changes to retain the backward and forward compatibility of the protocol.
Now, we can always decide to make a breaking change and override the workflow. This is the reason why I set this up as it's own part of the Workflow. If everything else is green and only the Buf check is read someone with the power to merge can decide to simply merge the change.
Since we always compare against master from then on the build will be green again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1059730693
##########
connector/connect/dev/detect_breaking_changes.sh:
##########
@@ -0,0 +1,36 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+if [[ $# -gt 0 ]]; then
+ echo "Illegal number of parameters."
+ echo "Usage: ./connector/connect/dev/detect_breaking_changes.sh"
+ exit -1
+fi
+
+
+SPARK_HOME="$(cd "`dirname $0`"/../../..; pwd)"
+cd "$SPARK_HOME"
+
+pushd connector/connect/common/src/main
+
+buf breaking --against "https://github.com/apache/spark/archive/master.zip#strip_components=1,subdir=connector/connect/common/src/main"
Review Comment:
Just noticed that it will have a problem after we release 3.4. For example, let's say we added a new protobuf message in Spark 3.5, and if a followup fixes the protobuf definition, this check will fail.
Can we probably use a different status of the GitHub check instead of a failure? The problem here is that it can be false negative that could confuse the developers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1059728218
##########
connector/connect/dev/detect_breaking_changes.sh:
##########
@@ -0,0 +1,36 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+if [[ $# -gt 0 ]]; then
+ echo "Illegal number of parameters."
+ echo "Usage: ./connector/connect/dev/detect_breaking_changes.sh"
+ exit -1
+fi
+
+
+SPARK_HOME="$(cd "`dirname $0`"/../../..; pwd)"
+cd "$SPARK_HOME"
+
+pushd connector/connect/common/src/main
+
+buf breaking --against "https://github.com/apache/spark/archive/master.zip#strip_components=1,subdir=connector/connect/common/src/main"
+if [[ $? -ne -0 ]]; then
+ echo "Buf detected breaking changes for your Pull Request. Please verify."
Review Comment:
The error message seems like it's a test failure. It is technically not a failure until we release Spark 3.4. Would probably have to make the error message different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1059728891
##########
connector/connect/dev/detect_breaking_changes.sh:
##########
@@ -0,0 +1,36 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+if [[ $# -gt 0 ]]; then
+ echo "Illegal number of parameters."
+ echo "Usage: ./connector/connect/dev/detect_breaking_changes.sh"
+ exit -1
+fi
+
+
+SPARK_HOME="$(cd "`dirname $0`"/../../..; pwd)"
+cd "$SPARK_HOME"
+
+pushd connector/connect/common/src/main
+
+buf breaking --against "https://github.com/apache/spark/archive/master.zip#strip_components=1,subdir=connector/connect/common/src/main"
Review Comment:
Wonder if we can use a plugin instead of adding this script, e.g., https://github.com/bufbuild/buf-breaking-action
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1059728156
##########
connector/connect/dev/detect_breaking_changes.sh:
##########
@@ -0,0 +1,36 @@
+#!/bin/bash
Review Comment:
Can we move this into `dev/` directory since all are placed there?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1367718329
For a breaking change which break the binary compatibility, we can add it to `MimaExcludes` after confirmation.
How can we make braking changes in proto? Is it forbidden?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1367741065
> generally, yes. We must not make breaking changes to retain the backward and forward compatibility of the protocol.
Just a side note, if I understand the forward compatibility right (a newer proto client talks to older proto server) here, the way of buf stops breaking change do not complete solve for that case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on a diff in pull request #39294: [SPARK-41537] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1059628374
##########
.github/workflows/build_and_test.yml:
##########
@@ -494,6 +495,43 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check:
+ needs: [precondition, infra-image]
Review Comment:
I can try without, my assumption was since this is very close to the Python build the `infra-image` is actually needed to make sure the environment is consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #39294:
URL: https://github.com/apache/spark/pull/39294#issuecomment-1478626808
The current level of proto compat required is the most strict one which requires to be fully compatible, even with the naming of the fields. Strictly speaking, this is not required to be wire compatible. But before we relax the compatibility, I would like to enforce the most strict one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on a diff in pull request #39294: [SPARK-41537][INFRA][TESTS] Github Workflow Check for Breaking Changes in Spark Connect Proto
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #39294:
URL: https://github.com/apache/spark/pull/39294#discussion_r1146164518
##########
.github/workflows/build_and_test.yml:
##########
@@ -493,6 +494,80 @@ jobs:
name: test-results-sparkr--8-${{ inputs.hadoop }}-hive2.3
path: "**/target/test-reports/*.xml"
+ proto-breaking-changes-check-master:
+ needs: [precondition]
+ if: always() && fromJson(needs.precondition.outputs.required).proto-breaking-changes-check == 'true'
+ name: Spark Connect proto backwards compatibility check (master)
+ continue-on-error: true
+ runs-on: ubuntu-22.04
+ env:
+ LC_ALL: C.UTF-8
+ LANG: C.UTF-8
+ PYSPARK_DRIVER_PYTHON: python3.9
+ PYSPARK_PYTHON: python3.9
+ steps:
+ - name: Checkout Spark repository
+ uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
+ repository: apache/spark
+ ref: ${{ inputs.branch }}
+ - name: Add GITHUB_WORKSPACE to git trust safe.directory
+ run: |
+ git config --global --add safe.directory ${GITHUB_WORKSPACE}
+ - name: Sync the current branch with the latest in Apache Spark
+ if: github.repository != 'apache/spark'
+ run: |
+ git fetch https://github.com/$GITHUB_REPOSITORY.git ${GITHUB_REF#refs/heads/}
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' merge --no-commit --progress --squash FETCH_HEAD
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' commit -m "Merged commit" --allow-empty
+ # Install the `buf` CLI
+ - uses: bufbuild/buf-setup-action@v1
+ with:
+ github_token: ${{ secrets.GITHUB_TOKEN }}
+ # Run buf breaking.
+ - uses: bufbuild/buf-breaking-action@v1
+ with:
+ input: connector/connect/common/src/main
+ against: 'https://github.com/apache/spark.git#branch=master,subdir=connector/connect/common/src/main'
Review Comment:
Unfortunately the buf build does not support warnings. However, the master check has continue on error set so it should be ignored, but you still get the error message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org