Store-native TTL management #34
Conversation
ghost
assigned 
|
Benchmark comparison between master and this branch. |
|
Comments:
vs. I've memprofiled this. Iterator + deletes + expiries + order of random values in that execution + compaction + prefetch = wild fluctuations in benchmarks. I might play around with disabling compaction in tests to get consistent readings, but there's nothing I can do about removing prefetch – as that is an iterator-specific property, and our |
|
@bigs – this should be ready for review ;-) |
|
benchmp between baseline (prior to refactorings) and this PR here: |
pstoreds/addr_book.go
Outdated
| @@ -60,7 +82,7 @@ func NewAddrBook(ctx context.Context, ds ds.TxnDatastore, opts Options) (*dsAddr | |||
|
|
|||
| // Stop will signal the TTL manager to stop and block until it returns. | |||
| func (mgr *dsAddrBook) Stop() { | |||
There was a problem hiding this comment.
can we delete this? it's not part of the interface, right?
There was a problem hiding this comment.
Yeah, not part of the interface, which is an omission IMHO.
pstoreds/addr_book.go
Outdated
| // Attempt to add the key. | ||
| if err = txn.Put(key, addrs[i].Bytes()); err != nil { | ||
| // format: bytes(ttl) || bytes(multiaddr) | ||
| value := append(ttlB, addrs[i].Bytes()...) |
There was a problem hiding this comment.
let's make a record type, i.e.
type AddrRecord struct {
Addrs []ma.Multiaddr
TTL time.Duration
}and either hand write serializer/deserializer or use gob (simplest). that way we can isolate serialization/deserialization
| if existed[i] { | ||
| switch mode { |
There was a problem hiding this comment.
the enum is so nice :D like this
|
Closes libp2p/go-libp2p#1702 |
| @@ -57,14 +57,14 @@ | |||
| }, | |||
| { | |||
| "author": "magik6k", | |||
| "hash": "QmUCfrikzKVGAfpE31RPwPd32fu1DYxSG7HTGCadba5Wza", | |||
| "hash": "QmaiEBFgkgB1wjrPRxru5PyXPEkx58WuMjXNaR1Q9QNRjn", | |||
| "name": "go-ds-badger", | |||
There was a problem hiding this comment.
depending on badger directly this far down is a bit weird. Especially since its not something we are tied to at this point (and may move away from)
There was a problem hiding this comment.
It's just used in tests to create test instances -- not in production code. Any other way you can think of?
There was a problem hiding this comment.
In Java, this dependency would be in test scope in maven or gradle. No such thing in Go / gx AFAIK.
There was a problem hiding this comment.
not a gx thing. go get by default won't download test dependency packages. The -t flag is required to download test deps.
There was a problem hiding this comment.
How difficult would it be to add basic ttl support to the in-memory datastore. It doesn't have to be fancy, it could literally be a:
- Check on read.
- Sweep every N minutes.
There was a problem hiding this comment.
You mean the datastore not the peerstore, right? It would be pretty straightforward, I'll work on it!
Eventually (when KeyBook and PeerMetadata are migrated to use the datastore), it may make sense to remove the in-memory implementation of the peerstore, and have the constructor delegate to a map-based datastore peerstore.
That's almost a tongue twister!
|
this is a tough one. we need the options constructor no matter what. i’ll
think on this
…On Tue, Sep 18, 2018 at 14:44 Raúl Kripalani ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In package.json
<#34 (comment)>
:
> @@ -57,14 +57,14 @@
},
{
"author": "magik6k",
- "hash": "QmUCfrikzKVGAfpE31RPwPd32fu1DYxSG7HTGCadba5Wza",
+ "hash": "QmaiEBFgkgB1wjrPRxru5PyXPEkx58WuMjXNaR1Q9QNRjn",
"name": "go-ds-badger",
It's just used in tests to create test instances -- not in production
code. Any other way you can think of?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#34 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AANBWlB4gQifXOs-ij5A9so29uqKG5QEks5ucT8ZgaJpZM4Wne_I>
.
|
|
@bigs what are you referring to with the last comment? |
|
the ds badger dependency. let’s just include t for now. not a huge deal and
we can update it when gx is updated to handle test deps.
…On Wed, Sep 19, 2018 at 05:46 Raúl Kripalani ***@***.***> wrote:
@bigs <https://github.com/bigs> what are you referring to with the last
comment?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#34 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AANBWtnByhQayjdUoRZHH1EO1dAFSZZOks5uchKPgaJpZM4Wne_I>
.
|
This PR replaces the bespoke TTL manager with store-native TTL management.
Notes:
Entries are now expired by the DB without notifying us. To keep the cache consistent, we now store the invalidation timestamp, calculated as the earliest expiration out of all multiaddrs. Before we return a cached entry, we check it's expiration and remove it if it's expired, falling through to the DB to repopulate the entry afresh.
To support the
UpdateAddrs()operation, which performs a conditional TTL update, we need to store the original TTL alongside the entry. Since the requirement is simple, I have not used anything fancy for sedes. I'm just packing the TTL (int64) + entry, by concatenating their byte strings.Cache keys are now
peer.ID, no longer stringing them.Our friend badger doesn't support sub-second TTLs, so I've had to adjust the time-dependent TTL tests to use above-second resolutions.
Last but not least, previously we had two methods on the
ttlManagerto update the expiration:insertOrExtendTTLandsetTTL:ttlExtendmode.ttlOverridemode.Both modes are represented by enum values.