Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge upstream master into horizon-v2.28.2 to facilitate the back-merge #5206

Closed
wants to merge 22 commits into from

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Feb 13, 2024

What

I took the following steps:

git switch master
git pull upstream master   # sync to stellar/go
git checkout horizon-v2.28.2
git switch -C v2.28.2-backmerge
git merge -S master
# <resolve all conflicts>
git commit -S
git push origin v2.28.2-backmerge

The rationale is self-explanatory: we need the patch release changes back in master.

Why

Self-explanatory. Replaces #5205.

This was a very precarious merge (lots of conflicts were resolved) so I'd appreciate multiple eyes on it for sanity checking.

tamirms and others added 22 commits December 20, 2023 09:30
…dger to be different phrase to avoid conflict with existing 'Processed ledger' log output from fsm (stellar#5155)
…ellar#5171)

* go mod tidy
* Add double-close protection
* Add request tracking when cache invokes upstream download
* Add cache hit tracking
* Move stat tracking to a separate file
* Modify test to track stats+integrity after caching
* Stop double-closing identical XDR stream readers
…lar#5178)

* Change logging provider to Horizon's default rather than logrus
* Add check for ledger state in txsub

* Add test for badSeq

* Fix failing unittest

* Update system_test.go

* Small changes

* Update main.go
* services/horizon: Fix claimable balance query
* Fix Soroban RPC image incompatibility
…stellar#5197)

* Add a `--history-archive-caching` flag (default=true) to toggle behavior
* Refactor to use a library: fscache
* Hook new metric into prometheus
* Add parallel read test to stress cache
* Add tests for deadlocking and other misc. scenarios
    git switch master
    git pull upstream master   # sync to stellar/go
    git checkout horizon-v2.28.2
    git switch -C v2.28.2-backmerge
    git merge -S master
    # <resolve all conflicts>
    git commit -S
    git push origin v2.28.2-backmerge
@Shaptic Shaptic requested review from sreuland, urvisavla, a team and tamirms February 13, 2024 22:07
Comment on lines +134 to +136
a.stats.incrementDownloads()
// // this is a query on the HA server state, not a data/bucket file download
// a.stats.incrementRequests()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a diff I'm a little worried about because I'm not sure which is correct. @sreuland you're on the git blame for this so can you help give some insight?

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh, this looks like a bad merge conflict resolution, that's not what 2.28.2 looks like in this area.

hmm, when i ported 2.28.0 and 2.28.1 branches back to master, I used interactive rebase instead of merge-master-to-rel-then-merge-rel-back-to-master and found that resulted in less conflicts and easier to understand the remaining ones which did arise from archive pool stuff of light horizon going into master but not not on 2.28 yet

it may be worth re-trying this merge of 2.28.2 to master with a rebase instead just to see - try create new branch off release branch, from that branch git rebase -i master choose just the new commits on the 2.28.2 branch, and see how conflicts look from there, should be a lot less, and should only see the commits relevant to the 2.28.2 show up in git compare to master, whereas this pr is showing a lot of commits

Copy link
Contributor Author

@Shaptic Shaptic Feb 14, 2024

Choose a reason for hiding this comment

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

It's not a bad resolution because I did it on purpose: you can see that on master it has the commented-out lines! I left them both in specifically because I wanted to ensure that the behavior preserved here (i.e. incrementing downloads) is actually the correct one! So can you confirm that? 😄

The merge actually went well otherwise! The conflicts were almost exclusively related to my code in #5197 so I had a good handle on how to resolve everything.

I think the reason why there are so many commits showing here is exactly because you did an interactive rebase, so it still thinks that those commits from .0 and .1 are different than the ones you merged into master (since history got modified). I'll try it via the rebase approach to see if it's cleaner but I also think it's fine as-is since the conflicts got worked out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call on the interactive rebase - it's a cleaner history! At the expense of authors getting a little murky but that's okay. Please review there, instead ➡️ #5210!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants