Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Jun 5, 2023

What changes were proposed in this pull request?

The pr aims to upgrade buf from 1.20.0 to 1.23.1

Why are the changes needed?

1.Release Notes:

2.The new version brings some bug fixed and improvment, as follow:

  • Fix issue where buf beta graph would not print modules within a workspace that
    had no dependencies or dependents.
  • Fix issue where buf beta graph would print warnings for missing dependencies
    that were actually present.
  • Fix issue where locally-produced images did not have module information if the corresponding
    module was stored in the new cache.
  • Remove buf beta registry template.
  • Remove buf beta registry plugin {create,deprecate,list,undeprecate,version} and replace with
    buf beta registry plugin {push,delete}.
  • Update buf beta price with the latest pricing information.

3.Manually test: dev/connect-gen-protos.sh, this upgrade will not change the generated files.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • Manually test
  • Pass GA

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Just a head-up. This is 4th commit since 5/1. Shall we hold on this dependency and do accumulated-update monthly. For example, shall we wait until July 1st, @panbingkun and @zhengruifeng ?

$ git log --oneline --since 2023-05-01 | grep 'Upgrade buf'
d61c77cac1 [SPARK-43890][CONNECT][BUILD] Upgrade buf to v1.20.0
e186e9dfbf [SPARK-43599][CONNECT][BUILD] Upgrade buf to v1.19.0
7992455de2 [SPARK-43401][CONNECT][BUILD] Upgrade buf to v1.18.0

@zhengruifeng
Copy link
Contributor

@dongjoon-hyun I am fine with holding on it until July 1st, buf release is a bit frequent.
We may also need to upgrade it to the latest version before 3.5 rc.

@dongjoon-hyun
Copy link
Member

Thank you. Yes, I agree with you. Since the feature freeze is July 16th, maybe after July 10th?

@panbingkun
Copy link
Contributor Author

@dongjoon-hyun Ok, Let's holding on it until July 1st.

@dongjoon-hyun
Copy link
Member

Thank you, @panbingkun . I'm fine any date after July 1st~ Feel free to proceed after than.

@zhengruifeng
Copy link
Contributor

Thank you. Yes, I agree with you. Since the feature freeze is July 16th, maybe after July 10th?

July 10th also works for me

@LuciferYang LuciferYang marked this pull request as draft June 8, 2023 09:05
@LuciferYang
Copy link
Contributor

I set this to draft first to avoid unexpected merging

@panbingkun panbingkun changed the title [SPARK-43974][CONNECT][BUILD] Upgrade buf to v1.21.0 [SPARK-43974][CONNECT][BUILD] Upgrade buf to v1.22.0 Jun 24, 2023
@panbingkun panbingkun changed the title [SPARK-43974][CONNECT][BUILD] Upgrade buf to v1.22.0 [SPARK-43974][CONNECT][BUILD] Upgrade buf to v1.23.0 Jun 30, 2023
@panbingkun panbingkun changed the title [SPARK-43974][CONNECT][BUILD] Upgrade buf to v1.23.0 [SPARK-43974][CONNECT][BUILD] Upgrade buf to v1.23.1 Jul 1, 2023
@panbingkun panbingkun marked this pull request as ready for review July 11, 2023 00:48
@panbingkun
Copy link
Contributor Author

panbingkun commented Jul 11, 2023

@dongjoon-hyun @zhengruifeng @LuciferYang Before we release the new Spark version, please let's review this PR again. 😄

@zhengruifeng
Copy link
Contributor

merged to master

@LuciferYang
Copy link
Contributor

late LGTM

@panbingkun
Copy link
Contributor Author

There seems to be a problem with this patch, I need a new followup to solve this problem
@zhengruifeng
image

image

@panbingkun
Copy link
Contributor Author

panbingkun commented Jul 11, 2023

@zhengruifeng Should we migrate or revert it?
FOLLOWUP PR: #41934

@zhengruifeng
Copy link
Contributor

@panbingkun I think this failure started before this PR got merged.

https://github.com/apache/spark/actions/runs/5511781438/jobs/10047815050

does this upgrade cause so many changes in generated codes? #41934

@LuciferYang
Copy link
Contributor

LuciferYang commented Jul 11, 2023

It's strange that the failure started after upgrading to sbt 1.9.2, which doesn't seem to be related ...

https://github.com/apache/spark/actions/runs/5511636288/jobs/10047488637

@panbingkun
Copy link
Contributor Author

panbingkun commented Jul 11, 2023

@zhengruifeng
Yes, when I executed sh dev/connect-gen-protos.sh locally, the automatically generated file in Python changed.
My local buf version is:
image

@zhengruifeng
Copy link
Contributor

@panbingkun it is weird since IIRC the CI in this PR passed.

but, if this upgrade cause such large changes in generated codes, let's revert it for now to avoid big changes before code freeze.

@panbingkun
Copy link
Contributor Author

In order to address the #41933 issue, the above PR has been reverted and now resubmitted.
#41937

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants