Split the is_importing stat in two #96

elhmn · 2025-01-17T08:08:41Z

We want to use two new different sockstats, import_skip_push_limit and import_soft_throttling which are respectively used to allow a higher push limit, and implement soft throttling policy for imports.

We still forward the is_importing stat for to skip push limit for backward compatibility. We plan to remove it on a subsequent PR.

We want to use two new different sockstats, `import_skip_push_limit` and `import_soft_throttling` which are respectively used to allow a higher push limit, and implement soft throttling policy for imports. We still forward the `is_importing` stat for to skip push limit for backward compatibility. We plan to remove it on a subsequent PR.

Copilot encountered an error and was unable to review this pull request. You can try again, by re-requesting a review.

migue

Would you mind adding an extra test in spokes-receive-pack/blob/main/internal/integration/integration_test.go?

elhmn · 2025-01-21T14:58:25Z

Would you mind adding an extra test in spokes-receive-pack/blob/main/internal/integration/integration_test.go?

Done, let me know what you think 👌

internal/integration/integration_test.go

elhmn · 2025-01-21T15:21:52Z

@migue have you seen this flake before ?

PATH="/home/runner/work/spokes-receive-pack/spokes-receive-pack/bin:/home/runner/go/bin:/opt/hostedtoolcache/go/1.21.13/x64/bin:/snap/bin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin" \
    GIT_CONFIG_SYSTEM="/home/runner/work/spokes-receive-pack/spokes-receive-pack/internal/integration/testdata/gitconfig" \
    go test -race -timeout 60s --tags=integration ./... 2>&1
?   	github.com/github/spokes-receive-pack	[no test files]
?   	github.com/github/spokes-receive-pack/cmd/spokes-receive-pack-networked-wrapper	[no test files]
?   	github.com/github/spokes-receive-pack/cmd/spokes-receive-pack-wrapper	[no test files]
?   	github.com/github/spokes-receive-pack/git/contrib/persistent-https	[no test files]
?   	github.com/github/spokes-receive-pack/internal/receivepack	[no test files]
ok  	github.com/github/spokes-receive-pack/internal/config	1.095s
ok  	github.com/github/spokes-receive-pack/internal/governor	(cached)
--- FAIL: TestHiderefsConfig (0.04s)
    parse.go:172: run git [init --bare /tmp/TestHiderefsConfig29[19](https://github.com/github/spokes-receive-pack/actions/runs/12890001755/job/35938562019?pr=96#step:7:20)271008/001]
    parse.go:176: hint: Using 'master' as the name for the initial branch. This default branch name
        hint: is subject to change. To configure the initial branch name to use in all
        hint: of your new repositories, which will suppress this warning, call:
        hint:
        hint: 	git config --global init.defaultBranch <name>
        hint:
        hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
        hint: 'development'. The just-created branch can be renamed via this command:
        hint:
        hint: 	git branch -m <name>
        Initialized empty Git repository in /tmp/TestHiderefsConfig2919271008/001/
    parse.go:172: run git [-C /tmp/TestHiderefsConfig2919271008/001 fetch /home/runner/work/spokes-receive-pack/spokes-receive-pack/internal/integration/testdata/remote/git-internals-fork.git refs/heads/main:refs/heads/main]
    parse.go:176: From /home/runner/work/spokes-receive-pack/spokes-receive-pack/internal/integration/testdata/remote/git-internals-fork
         * [new branch]      main       -> main
    parse.go:172: run git [-C /tmp/TestHiderefsConfig2919271008/001 update-ref refs/__transferhiderefs1/example/anything e589bdee50e39beac562[20](https://github.com/github/spokes-receive-pack/actions/runs/12890001755/job/35938562019?pr=96#step:7:21)c4b7a716225f79e3cf]
    parse.go:172: run git [-C /tmp/TestHiderefsConfig2919271008/001 update-ref refs/__transferhiderefs2/example/anything e589bdee50e39beac56[22](https://github.com/github/spokes-receive-pack/actions/runs/12890001755/job/35938562019?pr=96#step:7:23)0c4b7a716225f79e3cf]
    parse.go:172: run git [-C /tmp/TestHiderefsConfig2919271008/001 update-ref refs/__transferhiderefs2/exception/anything e589bdee50e39beac56220c4b7a716225f79e3cf]
    parse.go:172: run git [-C /tmp/TestHiderefsConfig2919271008/001 update-ref refs/__receivehiderefs1/example/anything e589bdee50e39beac56220c4b7a7162[25](https://github.com/github/spokes-receive-pack/actions/runs/12890001755/job/35938562019?pr=96#step:7:26)f79e3cf]
    parse.go:172: run git [-C /tmp/TestHiderefsConfig2919271008/001 update-ref refs/__receivehiderefs2/example/anything e589bdee50e39beac56220c4b7a716225f79e3cf]
    parse.go:172: run git [-C /tmp/TestHiderefsConfig2919[27](https://github.com/github/spokes-receive-pack/actions/runs/12890001755/job/35938562019?pr=96#step:7:28)1008/001 update-ref refs/__uploadhiderefs/example/anything e589bdee50e39beac56220c4b7a716225f79e3cf]
    parse.go:172: run git [-C /tmp/TestHiderefsConfig[29](https://github.com/github/spokes-receive-pack/actions/runs/12890001755/job/35938562019?pr=96#step:7:30)19271008/001 config transfer.hiderefs refs/__transferhiderefs1]
    parse.go:172: run git [-C /tmp/TestHiderefsConfig2919271008/001 config receive.hiderefs refs/__receivehiderefs1]
    parse.go:172: run git [-C /tmp/TestHiderefsConfig2919271008/001 config uploadpack.hiderefs refs/__uploadhiderefs]
    hiderefs_test.go:83: 
        	Error Trace:	/home/runner/work/spokes-receive-pack/spokes-receive-pack/internal/integration/hiderefs_test.go:83
        	Error:      	Received unexpected error:
        	            	expected capabilities on first line of ref advertisement "e589bdee50e[39](https://github.com/github/spokes-receive-pack/actions/runs/12890001755/job/35938562019?pr=96#step:7:40)beac56220c4b7a716225f79e3cf refs/__transferhiderefs2/exception/anything\n"
        	Test:       	TestHiderefsConfig
FAIL
FAIL	github.com/github/spokes-receive-pack/internal/integration	17.586s

migue · 2025-01-21T15:26:55Z

No, this is the first time I see this

elhmn · 2025-01-21T15:36:01Z

No, this is the first time I see this

Well it does not look like a flake, it failed every single time I retried 😞 . I will try to figure out why that's happening.

migue · 2025-01-21T15:42:15Z

No, this is the first time I see this

Well it does not look like a flake, it failed every single time I retried 😞 . I will try to figure out why that's happening.

I've tried to reproduce it locally, using both the main branch and your PR branch and I have not been able to reproduce the error. I need to leave in ~15 mins but if you want we can pair tomorrow morning and take a deeper look at it

elhmn · 2025-01-21T15:47:08Z

No, this is the first time I see this

Well it does not look like a flake, it failed every single time I retried 😞 . I will try to figure out why that's happening.

I've tried to reproduce it locally, using both the main branch and your PR branch and I have not been able to reproduce the error. I need to leave in ~15 mins but if you want we can pair tomorrow morning and take a deeper look at it

Same here, let's pair tomorrow 👌

elhmn · 2025-01-21T18:43:02Z

Am not sure why that's happening, but I wonder if the protocol allow the list of capabilities to be empty but we absolutely expect it to be set when we parse the ref advertisement pkt-lines. This protocol definition seem to indicate that it can't be empty.

  capability-list  =  capability *(SP capability)
  capability       =  1*(LC_ALPHA / DIGIT / "-" / "_")
  LC_ALPHA         =  %x61-7A

But this doc for protocol v2 shows that capabilities can be empty and on a separate line than those of the ref advertisement.

protocol-version = PKT-LINE("version 2" LF)
capability-list = *capability
capability = PKT-LINE(key[=value] LF)

migue · 2025-01-22T13:56:16Z

It fails in main, I am not sure why. I have tried to reproduce it locally, using the very same Git version that the CI is using and I am not able to reproduce it.

elhmn force-pushed the elhmn-split-the-is-importing-stat branch 2 times, most recently from 2bfa3e4 to 6a33470 Compare January 17, 2025 08:21

elhmn mentioned this pull request Jan 17, 2025

Dont use is importing for push limit #97

Draft

elhmn force-pushed the elhmn-split-the-is-importing-stat branch from 6a33470 to b720153 Compare January 17, 2025 17:09

elhmn force-pushed the elhmn-split-the-is-importing-stat branch from b720153 to aeb2528 Compare January 17, 2025 17:31

elhmn changed the title ~~wip: split the is_importing stat~~ Split the is_importing stat in two Jan 17, 2025

elhmn marked this pull request as ready for review January 17, 2025 20:33

Copilot bot review requested due to automatic review settings January 17, 2025 20:33

Copilot AI reviewed Jan 17, 2025

View reviewed changes

elhmn requested a review from a team January 17, 2025 20:45

migue reviewed Jan 17, 2025

View reviewed changes

Add integration tests

1b3e794

elhmn requested a review from migue January 21, 2025 14:57

migue previously approved these changes Jan 21, 2025

View reviewed changes

internal/integration/integration_test.go Outdated Show resolved Hide resolved

internal/integration/integration_test.go Outdated Show resolved Hide resolved

fix typo

4823e2a

elhmn dismissed migue’s stale review via 4823e2a January 21, 2025 15:15

elhmn requested a review from migue January 21, 2025 15:16

migue approved these changes Jan 21, 2025

View reviewed changes

elhmn requested a review from migue January 22, 2025 13:36

elhmn merged commit b977044 into main Jan 22, 2025
6 of 9 checks passed

elhmn deleted the elhmn-split-the-is-importing-stat branch January 22, 2025 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the is_importing stat in two #96

Split the is_importing stat in two #96

elhmn commented Jan 17, 2025 •

edited

Loading

migue left a comment

elhmn commented Jan 21, 2025

elhmn commented Jan 21, 2025

migue commented Jan 21, 2025

elhmn commented Jan 21, 2025

migue commented Jan 21, 2025

elhmn commented Jan 21, 2025

elhmn commented Jan 21, 2025 •

edited

Loading

migue commented Jan 22, 2025

Split the is_importing stat in two #96

Split the is_importing stat in two #96

Conversation

elhmn commented Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

migue left a comment

Choose a reason for hiding this comment

elhmn commented Jan 21, 2025

elhmn commented Jan 21, 2025

migue commented Jan 21, 2025

elhmn commented Jan 21, 2025

migue commented Jan 21, 2025

elhmn commented Jan 21, 2025

elhmn commented Jan 21, 2025 • edited Loading

migue commented Jan 22, 2025

elhmn commented Jan 17, 2025 •

edited

Loading

elhmn commented Jan 21, 2025 •

edited

Loading