fix running-bytes-limit and add test coverage #44
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
merlinran
force-pushed
the
merlin/bytes-limiter
branch
from
224772c
to
817531d
Compare
| @@ -590,5 +590,5 @@ func parseRunningBytesLimit(s string) (limiter.Limiter, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| return limiter.NewRunningTotalLimiter(d, nBytes), nil | |||
| return limiter.NewRunningTotalLimiter(nBytes, d), nil | |||
There was a problem hiding this comment.
The parameter is reordered unintentionally, but this reads better.
| Request(n uint64) bool | ||
| Commit(n uint64) | ||
| Withdraw(n uint64) | ||
| Request(id string, n uint64, period time.Duration) bool |
There was a problem hiding this comment.
Changed the interface so we can use the auction ID as ID here. Hope the "Request + Secure" logic is not that confusing.
| // if the caller commits some tokens more than once, | ||
| // this could happen. Add a guard here just to prevent | ||
| // the total from wraping around to a gigantic number. | ||
| rl.total = 0 |
There was a problem hiding this comment.
Removed the check here now we use ID to identify the entry.
| } | ||
| if rl.total+n > rl.limit { | ||
| return false | ||
| } | ||
| rl.total += n | ||
| rl.requested[id] = requested{ | ||
| n: n, | ||
| t: time.AfterFunc(expiration, func() { rl.Withdraw(id) }), |
There was a problem hiding this comment.
The idea is simple: requested tokens are withdrawn (removed from requested) if not being Secure-ed in time.
| @@ -331,59 +336,25 @@ func (s *Service) auctionsHandler(from core.ID, topic string, msg []byte) ([]byt | |||
| return nil, nil | |||
| } | |||
|
|
|||
| func (s *Service) winsHandler(from core.ID, topic string, msg []byte) ([]byte, error) { | |||
There was a problem hiding this comment.
simply moved down to below filterAuction.
| return fmt.Errorf("getting number of sealing sectors: %v", err) | ||
| } | ||
| if n > s.sealingSectorsLimit { | ||
| log.Errorf("fail to get number of sealing sectors, continuing: %v", err) |
There was a problem hiding this comment.
This is likely to happen, see filecoin-project/lotus#7078 (comment) . Need to fix later but for now just avoid this error from preventing bidding.
| DealDataDirectory: t.TempDir(), | ||
| } | ||
| _, err = service.New(config, store, lc, fc) | ||
| _, err := newService(t, func(config *service.Config) { |
| _, err = service.New(config, store, lc, fc2) | ||
| require.Error(t, err) | ||
| } | ||
|
|
||
| func TestBytesLimit(t *testing.T) { |
There was a problem hiding this comment.
This test is a bit fragile consider the timing of different activities and potential conflict when committing transactions. It's surprising to see badger store hits conflict too. Anyway, seems working with the current setup.
There was a problem hiding this comment.
might be worth trying a newer badger release. unless we updated since the start of bidbot, there have been a few releases for v3... latest release: https://github.com/dgraph-io/badger/releases/tag/v3.2103.1
| return nil, fmt.Errorf("setting proposal cid: %v", err) | ||
| } | ||
| // ready to fetch data, so the requested quota is actually in use. | ||
| s.bytesLimiter.Secure(prop.AuctionId) |
There was a problem hiding this comment.
This is the core change - secure the quota when start downloading, and other requested quotas will eventually expire, leave room for new auctions.
previously, if bidbot doesn't win an auction, the requested quota never gets cleared. this adds a way to expire the quota after a period of time, unless it's secured. Signed-off-by: Merlin Ran <merlinran@gmail.com>
merlinran
force-pushed
the
merlin/bytes-limiter
branch
from
817531d
to
3235602
Compare
| _, err = service.New(config, store, lc, fc2) | ||
| require.Error(t, err) | ||
| } | ||
|
|
||
| func TestBytesLimit(t *testing.T) { |
There was a problem hiding this comment.
might be worth trying a newer badger release. unless we updated since the start of bidbot, there have been a few releases for v3... latest release: https://github.com/dgraph-io/badger/releases/tag/v3.2103.1
previously, if bidbot doesn't win an auction, the requested quota never gets cleared. this adds a way to expire the quota after a period of time, unless it's secured.