Skip to content
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

feat: resource log #16522

Merged
merged 1 commit into from
Jan 23, 2020
Merged

feat: resource log #16522

merged 1 commit into from
Jan 23, 2020

Conversation

gavincabbage
Copy link
Contributor

@gavincabbage gavincabbage commented Jan 13, 2020

This change introduces a resource audit logger to track changes to resources in the system. The operations on tasks, buckets and organizations are augmented to use this logger.

No implementation is currently provided.

@gavincabbage gavincabbage changed the title [WIP] feat: resource log feat: resource log Jan 15, 2020
@gavincabbage gavincabbage force-pushed the feat/kv-log branch 2 times, most recently from f9214ac to 266bce6 Compare January 22, 2020 16:43
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff! I added some clarifying comments for future readers as to why I like this design based on the fact that we will be storing this data in a columnar store 👍

@@ -8,6 +8,21 @@ import (
icontext "github.com/influxdata/influxdb/context"
)

func TestGetAuthorizer(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

kv/bucket.go Outdated Show resolved Hide resolved
Comment on lines +159 to +166
func ErrInternalBucketServiceError(op string, err error) *Error {
return &Error{
Code: EInternal,
Msg: fmt.Sprintf("unexpected error in buckets; Err: %v", err),
Op: op,
Err: err,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great addition 🥇

if err := s.putBucket(ctx, tx, b); err != nil {
v, err := json.Marshal(b)
if err != nil {
return influxdb.ErrInternalBucketServiceError(influxdb.OpCreateBucket, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ❤️ the distinct errors

Comment on lines +20 to +35
type Change struct {
// Type of change.
Type ChangeType
// ResourceID of the changed resource.
ResourceID influxdb.ID
// ResourceType that was changed.
ResourceType influxdb.ResourceType
// OrganizationID of the organization owning the changed resource.
OrganizationID influxdb.ID
// UserID of the user changing the resource.
UserID influxdb.ID
// ResourceBody after the change.
ResourceBody []byte
// Time when the resource was changed.
Time time.Time
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to state the design here, for my own clarification and thinking aloud. tl;dr is this approach is sound 👍


To know the specific changes made to a resource over time, we read its entire history and look at each prior ResourceBody to determine the deltas. Knowing that we will store the change history in InfluxDB, this approach is more efficient. Why? Prior values will be in the same column as the ResourceBody field and very likely stored in the same TSM block, so will be decompressed together. 💯


For posterity, the alternative would be to include the PreviousResourceBody as an additional value as a separate column. Whilst this approach may be better for a row-based data store, it will require separate reads for a columnar-based store like InfluxDB.

💯

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.

None yet

2 participants