-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Statestore prototype #14079
Statestore prototype #14079
Conversation
// predefined errors | ||
|
||
var ( | ||
ErrResourceInUse = errors.New("resource in use") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported var ErrResourceInUse should have comment or be unexported
ed3606b
to
9651bbc
Compare
90565d9
to
d8c36de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urso code and direction looks good to me, we might want to add a few unit test around the code, I have added a few comments, suggestion around the code.
We will also need to address the hound's comments.
entry.value.mux.Lock() | ||
defer entry.value.mux.Unlock() | ||
|
||
invariant(entry.value.pending > 0, "there should be pending updates") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should it more often, especially on a beta in progress feature :)
// in case we miss an unlock operation (programmer error or panic that hash | ||
// been caught) we set a finalizer to eventually free the resource. | ||
// The Unlock operation will unsert the finalizer. | ||
runtime.SetFinalizer(res, (*Resource).finalize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never used finalizer in Golang before, I've used them in other languages. But after reading a bit more about them and in the context of usage we want to protect the storage as much possible of system and programmer errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Developers are still asked to unlock used resources. The finalizer is just a safety-net.
r.unlink() | ||
} | ||
return r.isLocked | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking maybe a Lock that accept a deadline, but we can do the same logic in the consumer with the tryLock().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TryLock is supposed to return immediately. I don't want to change Lock/Unlock as is, but some LockWithContext
would indeed be nice. E.g. we could unblock the call on shutdown.
Idea of TryLock is similar to current 'Finished' flag in filebeat. Assumed code pattern:
res, ok := store.TryLock(path)
if !ok {
return fmt.Errorf("file %v is collected by another input", path)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sound good, The current API allow for that flexibility so lets keep it simple and add if needed or event make function that wrap that behavior.
// | ||
// The update call is thread-safe, as the update operation itself is protected. | ||
// But data races are still possible, if any two go-routines update | ||
// an overlapping set of fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I that case, I think in this case the implementation recommendation would be to fetch entry merge fields and update in the lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A resource is just a 'key' for the store. But the key should be associated with some real-world resource like a file. Users wanting to collect a file must lock said file and only release the lock if the file should not be collected anymore.
The lock must be held for as long as the file is owned by the current process.
Like:
res, ok := store.TryLock(path)
if !ok {
return <error>
}
defer res.Unlock()
// update file meta:
meta := struct{
Path string
LastOpen time.Time
...
}{ path, time.Now(), ... }
res.Update(&meta)
for { <tail file> }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the file collection use-case a resource might be updated by a file watcher and an active reader concurrently. This is perfectly valid, as the input holds the lock. In order to have a safe-update, the different go-routines must never update the same set of fields. The file watcher should update file metadata only (e.g. file is renamed), while the active reader should only update the offset. If go-routines adhere to updating distinct set of fields, there is no race.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong radio button /facepalm
Unit tests will definitely come. As the interfaces of this layer is still somewhat in the flux I'd like to postpone these to a later PR. I'm planning to first stabilize all interfaces in the different layers + introduce some test support with reusable mocks for the registry itself (so different dependents can be tested more easily). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have posted a few comments. Are you going to add the remove functionality of the store in this PR?
No. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so far so good
ef8c2be
to
40bed88
Compare
Connector directly wraps a registry and gives access to many stores. Each store accessed will create a session. The registry.Store will be closed once all sessions are closed. Go routines can create pending update operations that will be applied asynchronously later to the persistent registry store. At the same time a go routine can close a store instance if the store is not needed anymore. So to keep the store alive for as long as is required to finally sync the in memory and on-disk state, we introduce the concept of store sessions. A session is active for as long as the Store instance is alive or there are any pending updates in the system. The underlying persistent store can be closed only once all sessions for said store are have been finished.
aa4dfe5
to
773556b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I did another review.
* State store prototype * Add statestore close and deactivate support * Introduce connector and store sessions Connector directly wraps a registry and gives access to many stores. Each store accessed will create a session. The registry.Store will be closed once all sessions are closed. Go routines can create pending update operations that will be applied asynchronously later to the persistent registry store. At the same time a go routine can close a store instance if the store is not needed anymore. So to keep the store alive for as long as is required to finally sync the in memory and on-disk state, we introduce the concept of store sessions. A session is active for as long as the Store instance is alive or there are any pending updates in the system. The underlying persistent store can be closed only once all sessions for said store are have been finished. * minor cleanups * review * vendor missing dependency
Requires: #14144
The change introduces the statestore package that provides some coordinated access to the registry file.
The store does not implement removal yet. I'm thinking to rather have some GC based approach. This would make it easier to clean stale entries of configs that have gone. Well, and I didn't want the PR to grow too much :)
Go doc: