Vlad, - Please show me if there is a simple fix. If that's the case, yes, I will revert this out from the master branch. That works for me. - If not, let's make a new PR. - If you feel this is an issue, let's start a vote. Let me know.
On Thu, 27 Mar 2025 at 00:13, Rozov, Vlad <vro...@amazon.com.invalid> wrote: > Every graduated from incubating Apache project has guards against what you > name “chaotic” and what other name breaking best development practices. > Such guards include JIRA, unit tests and PR review. Instead of reverting > commit, I would expect you to open JIRA and outline what is broken. If you > filed JIRA, please provide the link, if not, please open one. > > It took me 2 hours to fix Spark shells, so should you open JIRA instead of > spending time to identify the commit and reverting it, you will save time > as well. I’ll post fix once JIRA is open and I validate that my > understanding of what is broken is the same as yours. > > Thank you, > > Vlad > > On Mar 26, 2025, at 12:01 AM, Hyukjin Kwon <gurwls...@apache.org> wrote: > > Rozov, please test the patch, see if there is a relevant test or not, and > add a test if not there. If it is difficult to add a test, describe it in > the PR description, and how you manually tested. > This is what I think you need to do instead of reverting the revert. > Imagine that there are many of such PRs living in the master branch. > Thankfully this time you are alone but imagine many people do the same > things - it will be chaotic. To me, it took 4 hours to identify this single > commit. > > About the change itself, you need to fix SBT together, and it will need > the changes such as > https://github.com/apache/spark/commit/e927a7edad47f449aeb0d5014b6185ac36b344d0 > . > Should also test Spark shells, and describe how you tested it as well. > > This is what I expect: > - Please show me if there is a simple fix. If that's the case, yes, I will > revert this out from the master branch. That works for me. > - If not, let's make a new PR. > - If you feel this is an issue, let's start a vote. Let me know. > > > > On Wed, 26 Mar 2025 at 15:07, Reynold Xin <r...@databricks.com.invalid> > wrote: > >> Sorry Vlad - I disagree. Where is the simple fix? As a new contributor, >> you should not be coming in guns blazing blaming committers who are trying >> to keep the master branch sane and clean. >> >> On Tue, Mar 25, 2025 at 10:53 PM Rozov, Vlad <vro...@amazon.com.invalid> >> wrote: >> >>> There is a simple fix. This is exactly what I outlined in the e-mail. >>> Prior to reverting commit (on master) it was necessary to see if an easy >>> fix exists. The PR that introduced the error was merged into master 3 >>> weeks ago, so I still don’t get why it was reverted overnight. It was >>> also necessary to open JIRA and provide reproducible steps. I assume that >>> steps on the PR is what needs to be fixed, but it will be better to avoid >>> guessing. >>> >>> Thank you, >>> >>> Vlad >>> >>> On Mar 25, 2025, at 10:05 PM, Reynold Xin <r...@databricks.com.INVALID> >>> wrote: >>> >>> Is there a fix already available or a very simple fix a committer can >>> create quickly? If yes, we can merge the fix. If there isn't, for major >>> functionality breaking change, we should just revert. That's fairly basic >>> software engineering practices. >>> >>> >>> On Tue, Mar 25, 2025 at 9:53 PM Hyukjin Kwon <gurwls...@apache.org> >>> wrote: >>> >>>> With the change, the main entry points, Spark shalls, don't work and >>>> developers cannot debug and test. The snapshots become uesless. >>>> >>>> The tests passed because you did not fix SBT. It needs a larger change. >>>> >>>> Such change cannot be in the source. I can start a vote if you think >>>> this is an issue. >>>> >>>> >>>> On Wed, Mar 26, 2025 at 12:32 PM Rozov, Vlad <vro...@amazon.com.invalid> >>>> wrote: >>>> >>>>> This does not make any sense. >>>>> >>>>> 1. There are no broken tests introduced by >>>>> https://github.com/apache/spark/pull/49971 >>>>> 2. There are no JIRA filed for “the main entry point” >>>>> 3. “The main entry point” that does not have any unit test suggests >>>>> that it is not the main entry point. >>>>> 4. It is not practical to revert every commit that introduced some >>>>> functional issue or regression. If regression can’t be fixed, it is >>>>> reverted, if it can be reasonably fixed, the fix is applied to the >>>>> regression. Just as an example, >>>>> https://github.com/apache/spark/pull/42823 introduced a regression. >>>>> It was fixed by https://github.com/apache/spark/pull/50348. Nobody >>>>> asked for the revert. >>>>> 5. The way how revert was handled is not "Apache way”. >>>>> >>>>> It may be easier for you to handle it in a single PR, but it is easier >>>>> for me to provide PR on top of existing changes and it will also provide >>>>> JIRA and a commit that documents why there is an issue. Otherwise it will >>>>> be easier to introduce the error back again. >>>>> >>>>> Thank you, >>>>> >>>>> Vlad >>>>> >>>>> >>>>> On Mar 25, 2025, at 4:58 PM, Hyukjin Kwon <gurwls...@apache.org> >>>>> wrote: >>>>> >>>>> Rozov, this broke the main entry points of release, Spark shells. Even >>>>> in the mast branch, you build a Spark, and cannot use Spark shells. >>>>> Why don't you submit a PR that contains the proper fix? It is easier >>>>> to have one PR that has no issue, e.g., reverting backporting etc. >>>>> >>>>> On Wed, 26 Mar 2025 at 00:17, Rozov, Vlad <vro...@amazon.com.invalid> >>>>> wrote: >>>>> >>>>>> Hi All, >>>>>> >>>>>> I kind of understand why https://github.com/apache/spark/pull/49971 was >>>>>> reverted on the branch-4.0 to allow testing of 4.0 release. Why was it >>>>>> also >>>>>> reverted on the master branch? I don’t see any JIRA being open for the >>>>>> failure. AFAIK, the proper way to handle the issue in Apache project: >>>>>> >>>>>> - revert https://github.com/apache/spark/pull/49971 on the >>>>>> branch-4.0 (note that PR targeted master branch and SPARK-51229 targets >>>>>> 4.1 >>>>>> as affected version) >>>>>> - open JIRA and provide details of how to reproduce the error on the >>>>>> master branch >>>>>> - wait for the author of https://github.com/apache/spark/pull/49971 (in >>>>>> this case it was me) to reply in a timely manner and only then revert the >>>>>> commit >>>>>> >>>>>> If you agree with the approach, please undo the revert and file JIRA. >>>>>> There is an easy fix to the issue. >>>>>> >>>>>> Thank you, >>>>>> >>>>>> Vlad >>>>>> >>>>>> >>>>>> >>>>> >>> >
