Limit the number of duplicate requests in flight #445
Conversation
cmd/experimental/posix/main.go
Outdated
| @@ -168,7 +169,7 @@ func newStorage(ctx context.Context, signer note.Signer) (*storage.CTStorage, er | |||
| return nil, fmt.Errorf("failed to initialize GCP issuer storage: %v", err) | |||
| } | |||
|
|
|||
| return storage.NewCTStorage(ctx, appender, issuerStorage, reader, *enablePublicationAwaiter) | |||
| return storage.NewCTStorage(ctx, appender, issuerStorage, reader, *enablePublicationAwaiter, *pushbackMaxDupesInFlight) | |||
There was a problem hiding this comment.
Feels like this is getting close to warranting a CTStorageOptions struct
There was a problem hiding this comment.
+1 (it's getting longer and longer)
There was a problem hiding this comment.
Done. We now have to pass a struct to a function that itself create a new struct... We should be able to restructure the code a bit later, but let's leave it like this for now to see what we'll need to add / remove.
storage/storage.go
Outdated
| } | ||
|
|
||
| t := time.NewTicker(1 * time.Second) | ||
| go func() { |
There was a problem hiding this comment.
Might be worthwhile adding a small comment which just explains how this limiting works. It's straightfoward and easy to understand, but there's related action at a distance so worth a bread-crumb trail?
A vehicle for this could be to pull the inline func out into a ...Job func like we do in the Tessera storage impls and use the opportunity to pop on a Godoc func comment?
storage/storage.go
Outdated
| @@ -138,6 +155,10 @@ func (cts *CTStorage) Add(ctx context.Context, entry *ctonly.Entry) (uint64, uin | |||
| } | |||
| } | |||
| if idx.IsDup { | |||
| if cts.dupesInFlight.Load() > int64(cts.maxDupesInFlight) { | |||
| return 0, 0, fmt.Errorf("antispam in-flight %w", tessera.ErrPushback) | |||
There was a problem hiding this comment.
| return 0, 0, fmt.Errorf("antispam in-flight %w", tessera.ErrPushback) | |
| return 0, 0, fmt.Errorf("too many duplicate submissions %w", tessera.ErrPushback) |
cmd/experimental/posix/main.go
Outdated
| @@ -59,6 +59,7 @@ var ( | |||
| origin = flag.String("origin", "", "Origin of the log, for checkpoints and the monitoring prefix.") | |||
| storageDir = flag.String("storage_dir", "", "Path to root of log storage.") | |||
| pushbackMaxOutstanding = flag.Uint("pushback_max_outstanding", tessera.DefaultPushbackMaxOutstanding, "Maximum number of number of in-flight add requests - i.e. the number of entries with sequence numbers assigned, but which are not yet integrated into the log.") | |||
| pushbackMaxDupesInFlight = flag.Uint("pushback_max_dupes_in_flight", 100, "Maximum number of number of in-flight duplicate add requests. When 0, duplicate entries are always pushed back.") | |||
There was a problem hiding this comment.
I think we can remove _in_flight from the flag name. The pushback_max_outstanding flag is also for in-flight requests and we don't have "in flight" in the flag name.
There was a problem hiding this comment.
I thought a lot about this before sending this PR, and after reading your comment. pushbackMaxOutstanding has an element of time with "outstanding", implying that eventually there will be less outstanding requests at some point.
Given that this new pushback mechanism looks at the number of request happening at a given point in time, I think using the "inFlight" qualifier better conveys that this. I've changed the name, and improved the flag description.
storage/storage.go
Outdated
| dupesInFlight atomic.Int64 | ||
| maxDupesInFlight uint |
There was a problem hiding this comment.
| dupesInFlight atomic.Int64 | |
| maxDupesInFlight uint | |
| maxDupesInFlight uint | |
| dupesInFlight atomic.Int64 |
Separating the struct variables into two different groups helps the readability.
There was a problem hiding this comment.
I agree that we could probably re-arrange this struct. I can't really tell what grouping you were thinking of and why dupesInFlight should be alone though. I'll merge this as is, and we can talk about it when you're back :)
There was a problem hiding this comment.
Only dedupeFutureInFlight is not passed from NewCTStorage func. That's why I think it's better to put it in another group (or at least put it at the end of the list).
type CTStorage struct {
storeData func(context.Context, *ctonly.Entry) tessera.IndexFuture
storeIssuers func(context.Context, []KV) error
reader tessera.LogReader
awaiter *tessera.PublicationAwaiter
enableAwaiter bool
dedupeFutureInFlight atomic.Int64
maxDedupeFutureInFlight uint
} ctStorage := &CTStorage{
storeData: tessera.NewCertificateTransparencyAppender(opts.Appender),
storeIssuers: cachedStoreIssuers(opts.IssuerStorage),
reader: opts.Reader,
awaiter: awaiter,
enableAwaiter: opts.EnableAwaiter,
maxDedupeFutureInFlight: opts.MaxDedupeInFlight,
}
phbnf
force-pushed
the
deduplock
branch
5 times, most recently
from
e053f35 to
a49b784
Compare
|
I've changed this PR quite a lot:
|
# Conflicts: # cmd/tesseract/gcp/main.go
# Conflicts: # cmd/tesseract/gcp/main.go
Towards #322, #448