Hi, Just to add my +1 to Michael's analysis.
It does seem crave is doing a lot of extra work determining the list of correct changes as patches, when a single patch file would be sufficient to run the build. not completely sure if you need that entire metadata for any reason. I ran a quick test on one of my failing PRs and it does seem that using `git merge-base` finds a newer reference than what crave is showing in the logs. Looking at the crave logs a bit more, it also looks like the build is trying to apply the various PR commits in some time-respecting order mixing with existing commits on main branch and this can turn out to be inconsistent because locally changes conflict but globally the PR code could have evolved already to no longer conflict (change at a later time that solves this conflict) Example with my PR https://github.com/apache/solr/pull/1856 relevant logs are https://github.com/apache/solr/actions/runs/6039688107/job/16388913192?pr=1856 Doing a hard reset to what crave things it's the correct base: git reset --hard f6ef54a757cab2e3f4ee9785e234bff0bc5a22b0 Then cherry-pick all the way to the conflict, to follow crave logs Applying: Move Jetty HttpClient tracing into InstrumentedHttpListenerFactory Applying: Update software.amazon.awssdk:* to v2.20.128 (#1820) Applying: SOLR-16929 SolrStream propagates undecoded error message (#1852) Applying: SOLR-16944 V2 API /api/node/health should be governed by "health" permission, not "config-read" (#1858) Applying: Update v2-api.adoc: remove mention of /api/c (#1790) Applying: SOLR-16934: Give securityManager permission for client TLS (#1857) Applying: SOLR-16265: Fix NPE for req in Http2SolrClient (#1860) Applying: moved tracing to the instrumented factories. some more tweaks to make the tests pass Applying: SOLR-16946: Updated Cluster Singleton plugins are stopped correctly when the Overseer is closed (#1862) Applying: Cleaning up old code to prevent warnings (#1834) Applying: tidy Applying: SOLR-16933: Include full query response in API tool (#1863) Applying: SOLR-16933: Use JSONMapResponseWriter for CLI errors Applying: poc to reinstate InheritableThreadLocalProvider and auto gen a span if one doesn't exist already Applying: SOLR-16825: Migrate v2 definitions to 'api' module (#1859) Applying: SOLR-16916: JSON boolean queries when solrconfig defType is set to edismax (#1827) Applying: SOLR-15367 Convert "rid" functionality into a default Tracer (#1854) Patch failed at 0017 SOLR-15367 Convert "rid" functionality into a default Tracer (#1854) and yes I can repro the conflict, this is the last cherry pick: $ git cherry-pick ce6c13e41806233daaf25f1a7545c7aad34c169d Auto-merging solr/core/src/java/org/apache/solr/core/TracerConfigurator.java CONFLICT (content): Merge conflict in solr/core/src/java/org/apache/solr/core/TracerConfigurator.java Auto-merging solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java error: could not apply ce6c13e4180... SOLR-15367 Convert "rid" functionality into a default Tracer (#1854) hint: After resolving the conflicts, mark them with hint: "git add/rm <pathspec>", then run hint: "git cherry-pick --continue". hint: You can instead skip this commit with "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort". Recorded preimage for 'solr/core/src/java/org/apache/solr/core/TracerConfigurator.java' This is related to some changes I did in my own PR which got refactored in the end. so globally consistent but locally not. Also one minor observation, it looks like the `Run actions/checkout@v3` is taking ~1minute and 'm wondering if this is related to the fact that we are using `fetch-depth: 0` in our settings, when we don't necessarily need all the branches, or do we? hope this help clarify things a bit more, alex On Thu, Aug 31, 2023 at 9:29 AM Michael Gibney <mich...@michaelgibney.net> wrote: > I realized that although I had responded on Aug. 24, the response was > sent only to Uv, not to the list. The commits on the PR referenced > have changed now, since I went ahead and rebased for this PR in order > to be able to merge it. But the response should be applicable to other > PRs as well. And in fact, the changed commits are themselves a good > example of why it would be good to change this workflow so that it > doesn't effectively require force-pushes! > > My response from Aug. 24: > > Thank you for your considered response! Having the specific commands > is helpful. Taking https://github.com/apache/solr/pull/1351 as a > concrete example, I wonder whether there's a reason that you don't use > the more straightforward `git merge-base` command for determining the > merge base? > Running the `git rev-list` command you provided results in > 14dd04903a68f69dcfc2104fc70a8ff1c44986b3^ as the merge base, whereas > the more straightforward `git merge-base <PR_HEAD> <UPSTREAM_HEAD>` > results in 9c43fc138b04c1420b912ec4371eea77335f905b, which is in fact > the "merge base", according to the usual git semantics. > > Using the latter commit with format-patch command you supplied in this > case generates a sequence of 5 patches that don't apply cleanly. Using > the other commit (the one that crave.io is currently using) with the > format-patch command generates a sequence of 455 patches, which don't > apply cleanly. But even if they did apply cleanly, with 455 separate > commits it's clear (unless I'm missing something) that in this case > the goal of minimizing bandwidth and achieving incremental updates is > not being achieved. > > What I'm suggesting is that in step 2.2, rather than using `git > format-patch`, using `git diff > 9c43fc138b04c1420b912ec4371eea77335f905b..<PR_HEAD>` (where > 9c43fc138b04c1420b912ec4371eea77335f905b is the "merge base" according > to the usual git semantics) would generate a single patch (albeit > without the irrelevant commit metadata) that is guaranteed to apply > cleanly if the PR_HEAD has recently merged from (or rebased onto) > upstream head. > > I'm also a bit confused -- in 2.2, you state that you're using `git > format-patch` to generate the patches, but subsequently you say "we > don't use format-patch"? > > On Tue, Aug 22, 2023 at 8:54 PM Yuvraaj Kelkar <yuvr...@gmail.com> wrote: > > > > Sorry for the delay, I had to collect this information from the team. > > > > As you said: Crave doesn't need to know the commit history, it just > needs enough information from git to be able to reproduce the filesystem > state on to the remote side without having to do a full rsync. > > We have to account for the possibility that there's any combination of: > (common merge base whether on branch or not) + (staged commits) + (unstaged > commits) > > > > Patch process: > > > > Get the merge base > > > > Use the command git rev-list --topo-order --reverse <localBranch> --not > --remotes=<remote> -- to get a list of commit ID. > > If the list is empty, then the HEAD commit in the branch is the common > merge base. Use it. > > If the list is NOT empty, then use the first entry in the list to get > one commit ID before it. This is the merge base. > > > > With the merge base, creare a series of patches > > > > Staged changes: git --no-pager diff --src-prefix=a/ --dst-prefix=b/ > --binary --no-ext-diff --ignore-submodules --no-color --cached > > Local changes: git --no-pager format-patch --src-prefix=a/ > --dst-prefix=b/ --no-color --stdout <mergeBase> > > > > Send patches list across to the remote side, and then apply using git > apply --verbose --whitespace=nowarn <patch> > > > > In the past, we did use git am , but it is all git apply now. > > Also, we don't use format-patch because we had to implement > "incremental git patching" for users repeatedly doing incremental builds in > the common case, eg: > > User does incremental builds as changes keep getting "collected". At the > end of 3-4 days, there was 200 files modified. Patching 200 files on every > incremental caused a mtime update that causes the build system from > unnecessarily rebuilding far more than actually needed. > > A simple git diff allows us to do incremental diff patching. > > > > But besides all this explanation, we're looking to improve the entire > patching process so that there's no need to repeatedly restart Github > Actions. > > One of the ways we're trying to get debug information is to get access > to the Action Runner so that we can look at the git tree so we can get a > repro without having to guess every time. > > > > Thanks, > > -Uv > > > > On Aug 2 2023, at 10:50 am, Michael Gibney <mich...@michaelgibney.net> > wrote: > > > > Hi Uv: > > > > regarding > https://lists.apache.org/thread/l8hgqwkm9vt8yhygoo2blw5j4n5gh121 > > > > Would it be possible for Crave, instead of using `git format-patch` / > > `git am`, to instead generate/apply a simple single diff/patch, via > > `git diff <merge-base>..<pr_head> > file.patch` / `git apply > > file.patch`? > > > > This would address a couple of problems, and should be more efficient > > on IO as well (which is I gather the motivation for the patch-based > > approach in the first place). > > > > In some cases (e.g., https://github.com/apache/solr/pull/1351), Crave > > fails to apply patches even when the source PR branch is completely > > up-to-date (has merged in the latest commit from `main`). The > > workaround of rebasing is not ideal, in that it necessitates > > force-pushes, which are considered an anti-pattern in some contexts, > > and can generally be an inconvenience for transparency and > > collaborative development. > > > > But separately relevant: Crave doesn't care about the commit history > > -- it's only using patches to optimize IO bandwidth. So generating a > > single diff between the merge-base and the PR head should iiuc be > > exactly what you want, and would be guaranteed to always apply > > cleanly. > > > > Would you mind sharing how you're deriving the "merge-base" commit, > > and the command you're using (I assume, `git format-patch`?) to > > generate the patch files? Having just merged `main` into a PR branch, > > the merge-base of the PR branch and `main` should be: `main`. > > > > Thank you for your consideration! > > Michael > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@solr.apache.org > For additional commands, e-mail: dev-h...@solr.apache.org > >
