-
Notifications
You must be signed in to change notification settings - Fork 187
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
Activity API #9361
Activity API #9361
Conversation
Question: Do we use the generated libregraph types? |
No. We are barely using them. No need to use created ones imho. |
I think we have a problem. This api endpoint needs to work with our generated clients. Is the spec already added to libregraph? |
Yes. Then we really have a problem. We need to discuss this. |
What needs to be discussed? We are using libregraph. So our normal workflow is to create the spec and then the implementation. Why should it be different here? |
Well the only thing we have in common with the graph spec is the url. We are returning a different response and expect a different request. That is because we are misusing the endpoint as it doesn't exist as we need it to be. |
332fdb6
to
a18ac33
Compare
if err := json.Unmarshal(records[0].Value, &activities); err != nil { | ||
return nil, fmt.Errorf("could not unmarshal activities: %w", err) | ||
} | ||
|
||
return activities, nil | ||
} | ||
|
||
// RemoveActivities removes the activities from the given resource |
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.
Shouldn't we add a mutex to the ActivitylogService structure?
We are consuming and writing the events in Run() and at the same time we are doing threads unsafe read
write
sequence in a RemoveActivities
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.
Yes you are right. But adding a mutex wont fix the problem. In a distributed environment there might be multiple activitylog services running 🤔 I'll add a mutex for now, but we need a better solution.
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.
Now it is deadlock
You locked for the r/w a.lock.Lock()
in RemoveActivities and after that try to a.lock.RLock()
in Activities with waits forever
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.
The proper solution could be to split the Activities
method and use the private in case if we have to read write
func (a *ActivitylogService) Activities(rid *provider.ResourceId) ([]RawActivity, error) {
a.lock.RLock()
defer a.lock.RUnlock()
return s.activities(rid)
}
func (a *ActivitylogService) activities(rid *provider.ResourceId) ([]RawActivity, error) {
resourceID := storagespace.FormatResourceID(*rid)
records, err := a.store.Read(resourceID)
if err != nil && err != microstore.ErrNotFound {
return nil, fmt.Errorf("could not read activities: %w", err)
}
if len(records) == 0 {
return []RawActivity{}, nil
}
var activities []RawActivity
if err := json.Unmarshal(records[0].Value, &activities); err != nil {
return nil, fmt.Errorf("could not unmarshal activities: %w", err)
}
return activities, nil
}
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.
Looks good for me
if err := json.Unmarshal(records[0].Value, &activities); err != nil { | ||
return nil, fmt.Errorf("could not unmarshal activities: %w", err) | ||
} | ||
|
||
return activities, nil | ||
} | ||
|
||
// RemoveActivities removes the activities from the given resource |
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.
Now it is deadlock
You locked for the r/w a.lock.Lock()
in RemoveActivities and after that try to a.lock.RLock()
in Activities with waits forever
@kobergj mind to fix a typo from your former ActivityLog PR that would fit in here: |
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
- make use of libregraph artifacts - add a basic activity kql ast parser
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Only rebased to master - no code changes |
mux *chi.Mux | ||
evHistory ehsvc.EventHistoryService | ||
valService settingssvc.ValueService | ||
lock sync.RWMutex |
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.
It is a nitpick but could we use mu sync.RWMutex
which is the de facto standard?
Nevermind
Quality Gate passedIssues Measures |
Adds an API to retrieve activities
Fixes #9196