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

Make GitPackCache include ObjectType #942

Merged
merged 3 commits into from Sep 14, 2023

Conversation

georg-jung
Copy link
Contributor

See #876 (comment)

This adds caching of the determined objectType alongside the object's content. While the approach in d0aa014 seems a bit more straight forward than the one in fafa498, I think it is preferrable to have the contentType as part of the cached value instead of the cache key. Otherwise, repeated requests with a mismatching objectType will not profit from caching.

I thought about adding proper TryGetObject implementations as part of this. This turned out to be quite hard to get right though. The topics aren't too closely related too so I think they work as seperate PRs. Because these cache improvements should be sufficient to make #876 work, I decided to split this up and postpone the proper TryGetObject implementations.

Otherwise, we would have I/O operations for every request for a wrong objectType. By having it as part of the value we can use the cache and aquire the information that the objectType does not match from the cache.
@georg-jung georg-jung force-pushed the fix/cache-tryopen-objecttype branch from 3e57658 to 820e843 Compare July 10, 2023 11:07
@georg-jung
Copy link
Contributor Author

@AArnott any thoughts on this? I got some time in the coming weeks to work on this or #876 if there's something to improve (if this was merged, #876 would need to be rebased obviously).

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Looks fantastic! Thank you.

@AArnott
Copy link
Collaborator

AArnott commented Sep 14, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AArnott
Copy link
Collaborator

AArnott commented Sep 14, 2023

Sorry it took so long for me to get to this, @georg-jung.

@AArnott
Copy link
Collaborator

AArnott commented Sep 14, 2023

So weird.... I've re-queued the build 3 times and the test process crashes every time. Yet several other builds today passed the first time, so there seems to be something uniquely about this PR that causes a test crash, but it sure seems safe enough. I'm still looking into it.

@georg-jung
Copy link
Contributor Author

georg-jung commented Sep 14, 2023

Thanks for taking a look!

Back then, my pipeline ran. I started a run on my ADO on this PR's branch here: https://dev.azure.com/georg-jung/Nerdbank.GitVersioning/_build/results?buildId=1871&view=results

I also rebased the PR's contents in a second branch to be on-par with latest main and started a run on it too: https://dev.azure.com/georg-jung/Nerdbank.GitVersioning/_build/results?buildId=1872&view=results

Both are still in progress as of writing.

(Well the pipeline did/does fail, but that's because it can't publish packages to your CI feed; the tests ran successfully)

@georg-jung
Copy link
Contributor Author

All the test steps seem to work in my ADO, for the rebased version as well as the original one. Really strange.

@AArnott
Copy link
Collaborator

AArnott commented Sep 14, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AArnott AArnott merged commit f0abf4e into dotnet:main Sep 14, 2023
14 checks passed
@georg-jung georg-jung deleted the fix/cache-tryopen-objecttype branch September 15, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants