Skip to content

Conversation

@roger2hk
Copy link
Contributor

Towards #313.

@roger2hk roger2hk requested review from AlCutter and phbnf May 16, 2025 09:10
@AlCutter
Copy link
Collaborator

I think adding weird vendor specific URL scheme support to the core of the client is a bit of a slippery slope. How about if we at least put the GSFetcher into its own package somewhere, so folks using the client won't depend on that package unless they actively import & use it to create fetchers?

We could also tidy up the hammer API a bit to make it easier for users of that package to decide which fetchers to use - e.g. loadtest.NewLogClient could be deleted and replaced with e.g. loadtest.NewRoundRobinFetcher(f []Fetcher) Fetcher and loadtest.NewRoundRobinLeafWriter(w []LeafWriter) LeafWriter. This would perhaps fit a bit more naturally with the existing signature of loadtest.NewHammer which tries to be agnostic about how fetchers and writers are actually implemented.

wdyt @roger2hk ?

@AlCutter
Copy link
Collaborator

(e.g. transparency-dev/tessera#647)

@roger2hk
Copy link
Contributor Author

I think adding weird vendor specific URL scheme support to the core of the client is a bit of a slippery slope. How about if we at least put the GSFetcher into its own package somewhere, so folks using the client won't depend on that package unless they actively import & use it to create fetchers?

We could also tidy up the hammer API a bit to make it easier for users of that package to decide which fetchers to use - e.g. loadtest.NewLogClient could be deleted and replaced with e.g. loadtest.NewRoundRobinFetcher(f []Fetcher) Fetcher and loadtest.NewRoundRobinLeafWriter(w []LeafWriter) LeafWriter. This would perhaps fit a bit more naturally with the existing signature of loadtest.NewHammer which tries to be agnostic about how fetchers and writers are actually implemented.

wdyt @roger2hk ?

Sounds good to me. I've created a gcp package and moved GSFetcher into it. Thanks for the refactoring in Tessera hammer.

@AlCutter
Copy link
Collaborator

I think adding weird vendor specific URL scheme support to the core of the client is a bit of a slippery slope. How about if we at least put the GSFetcher into its own package somewhere, so folks using the client won't depend on that package unless they actively import & use it to create fetchers?
We could also tidy up the hammer API a bit to make it easier for users of that package to decide which fetchers to use - e.g. loadtest.NewLogClient could be deleted and replaced with e.g. loadtest.NewRoundRobinFetcher(f []Fetcher) Fetcher and loadtest.NewRoundRobinLeafWriter(w []LeafWriter) LeafWriter. This would perhaps fit a bit more naturally with the existing signature of loadtest.NewHammer which tries to be agnostic about how fetchers and writers are actually implemented.
wdyt @roger2hk ?

Sounds good to me. I've created a gcp package and moved GSFetcher into it. Thanks for the refactoring in Tessera hammer.

Will you make the same changes to the fork here in this PR, or as a follow-up?

@roger2hk
Copy link
Contributor Author

I think adding weird vendor specific URL scheme support to the core of the client is a bit of a slippery slope. How about if we at least put the GSFetcher into its own package somewhere, so folks using the client won't depend on that package unless they actively import & use it to create fetchers?
We could also tidy up the hammer API a bit to make it easier for users of that package to decide which fetchers to use - e.g. loadtest.NewLogClient could be deleted and replaced with e.g. loadtest.NewRoundRobinFetcher(f []Fetcher) Fetcher and loadtest.NewRoundRobinLeafWriter(w []LeafWriter) LeafWriter. This would perhaps fit a bit more naturally with the existing signature of loadtest.NewHammer which tries to be agnostic about how fetchers and writers are actually implemented.
wdyt @roger2hk ?

Sounds good to me. I've created a gcp package and moved GSFetcher into it. Thanks for the refactoring in Tessera hammer.

Will you make the same changes to the fork here in this PR, or as a follow-up?

I'm going to propagate the changes in a separate PR after the one in Tessera is merged.

@roger2hk roger2hk merged commit 2911aed into transparency-dev:main May 16, 2025
7 checks passed
@roger2hk roger2hk deleted the hammer-client-gs-fetcher branch May 16, 2025 14:00
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.

3 participants