Add apistore for caching the latest events #1429

nakabonne · 2021-01-14T06:37:40Z

What this PR does / why we need it:
This PR adds a new apistore for Events according to @nghialv 's suggestion. It allows us to reduce communication number by caching the latest events.

This will be used by #1411 at first.

Which issue(s) this PR fixes:

Ref #1381

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2021-01-14T06:37:42Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Use UTC and let control-plane convert it into the control-plane's Local time

https://github.com/pipe-cd/pipe/blob/ab31e9a63e0bab6591e8c572bc06f982dc34ea32/pkg/app/piped/apistore/eventstore/store.go#L104-L107

This was created by todo plugin since "TODO:" was found in `ab31e9a` when #1429 was merged. cc: @nakabonne.

nakabonne · 2021-01-14T06:39:51Z

pkg/app/piped/apistore/eventstore/store.go

+	// TODO: Use UTC and let control-plane convert it into the control-plane's Local time
+	// Unexpected behavior can be happened if Piped uses different timezone from the control-plane.


I will look into this concern when working on another PR.

I think the timezone difference is not a problem, because we are using Unix time (the number of seconds elapsed since January 1, 1970 UTC.).
But the clock drift may cause the problem. But that is rare.

Btw, maybe we can leave the to parameter as zero to use time.Now of the server.

I think the timezone difference is not a problem, because we are using Unix time (the number of seconds elapsed since January 1, 1970 UTC.).

I put off thinking about it but after thinking, it's a basic thing and no problem as you mentiond.

Btw, maybe we can leave the to parameter as zero to use time.Now of the server.

Cool, I'll try it.

Btw, maybe we can leave the to parameter as zero to use time.Now of the server.

After thinking, it makes derivating the next's from tougher. As you mentioned, clock drifts is reraly happened. I guess it's enough to be as-is.

Not tougher, I came up with a better solution just now. I'm going to make From and To optional.

pipecd-bot · 2021-01-14T06:43:28Z

Code coverage for golang is 33.89%. This pull request decreases coverage by -0.07%.

File	Function	Base	Head	Diff
pkg/app/piped/apistore/eventstore/store.go	NewStore	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Run	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.sync	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Getter	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.GetLatest	`--`	`0.00%`	`+0.00%`
pkg/model/event.go	EventDefinitionID	`--`	`100.00%`	`+100.00%`
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ListEvents	`0.00%`	`0.00%`	`+0.00%`

nakabonne · 2021-01-14T06:45:04Z

pkg/model/event.go

+// EventDefinitionID builds a unique identifier based on the given name and labels.
+// It returns the exact same string as long as both are the same.
+func EventDefinitionID(name string, labels map[string]string) string {


I've been thinking about what we should call something whose uniqueness is consists of its Name and Labels. I just settled on calling it Event Definition.

An Event is like an Instance In Object-oriented world, and an event definition is like a Class. Each Event has its own ID and at the same time, each Event definition has its own ID. The event definition ID must be the exact same string as long as Name and Labels are the same.

I just figured out it would be nice if we build this when saving and keep it as a field of Event.

nghialv · 2021-01-14T06:59:28Z

pkg/model/event.go

+
+// EventDefinitionID builds a unique identifier based on the given name and labels.
+// It returns the exact same string as long as both are the same.
+func EventDefinitionID(name string, labels map[string]string) string {


This will be used only by EventStore right. So I think it should be moved to that package as an internal function.
So because of an internal function, we will not have to care about its name.

Actually it will be used by #1406. It could be increased the need to build a unique identity made of Name and Labels, that's why I decided to make a helper function.

I see. Now we are having our cache/store for the event. So maybe we don't need to merge them for simplicity.

The user can merge by changing their configuration if needed.
We implicitly merge them and combine their changes into a single commit that may cause the behavior they were not expected.
(I am not sure that I understood the merging correctly.)

(I am not sure that I understood the merging correctly.)

Your understanding is correct. I attempted to merge it to combine their changes into a single commit.

Okay, I agree with all of them. So let's this function private and think it if needed in the future!

nghialv · 2021-01-14T07:06:07Z

pkg/app/piped/apistore/eventstore/store.go

+		return nil, false
+	}
+
+	// NOTE: Don't update the cache to prevent it from being overwritten by slightly older ones.


Why we don't update the in-memory map?
The event has CreatedAt field, so we can use that to check whether the event should be updated or not.

And I think in the microservices list of our users, there are always some applications that have not been actively developed compared to others. Their events will be very sparse. So without saving into the list, piped will always make the call to control-plane to get the same one.

The sync function periodically updates the cache, and it works in another goroutine. What I'm most worried about is, while processing GetLatest, sync can update the cache. I mean:

// NOTE: Don't update the cache to prevent it from being overwritten by slightly older ones. eventMap = s.latestEventMap.Load().(map[string]*model.Event) eventMap[id] = resp.Event // I'm most worried about the "sync()" function updates the cache at this point. s.latestEventMap.Store(eventMap) // Then the latest cache is overwritten by the old one here.

If Piped was run on the multi-core processor, I think it can happen, but let me know if I'm wrong.

The data race can be resolved in some ways:

change from using atomic to using a mutex. (fetch the list from control-plane, lock, and update). I think it is fine enough for our case: 1 call/m and EventWatcher.

use 2 separate maps, one for LIST API, one for GET API

Ah, not fancy 😄 The first one looks enough! Only two goroutines can access this and it rarely happens to wait for unlocking. Thank you for telling me!

nghialv · 2021-01-14T07:08:18Z

pkg/app/api/service/pipedservice/service.proto

+    NONE = 0;
+    FROM_OLDEST = 1;
+    FROM_NEWEST = 2;
+}


How about ListOrder enum that contains NONE, DESC, ASC?

That sounds good, that's more versatile. I initially thought about making it but I wasn't sure which should be general, CreatedAt or UpdatedAt, that's why I made a special enum for now. but what do you think?

I think it is fine to make ListOrder. It can be used by other RPCs too.
Let's do it.

Fine, I'll do so!

nghialv · 2021-01-14T07:42:39Z

pkg/app/piped/apistore/eventstore/store.go

+func (s *store) GetLatest(ctx context.Context, name string, labels map[string]string) (*model.Event, bool) {
+	eventMap := s.latestEventMap.Load().(map[string]*model.Event)
+	id := model.EventDefinitionID(name, labels)
+	event, ok := eventMap[id]


One thing I want to ask about our event matching, in case we have an event

name = foo labels: { label-1: value-1, label-2: value-2}

That event will match the following case or not?

piVersion: pipecd.dev/v1beta1 kind: EventWatcher spec: events: - name: foo labels: label-1: value-1 replacements: - file: dev/app1/deployment.yaml yamlField: $.spec.template.spec.containers[0].image

Or all labels must be specified to be matched.

I totally assume exact matches are needed, but on second thought, seems like that case should be treated as a matching case. Let me think, just a moment please...

@nghialv I'm still debating but which one do you prefer?

If possible, the event should be matched when the name is equal AND the event's labels contain the specified ones.
(Because they are labels (not ID) and users will have more options.)

So I think we can change the map to a new data structure like map[event-name]ListOfEventsHaveSameName.

But one thing we have to consider is what happens when multiple events match the specified label list.

But one thing we have to consider is what happens when multiple events match the specified label list.

So in order to avoid this complexity and avoid adding the dynamic label by users (e.g. version number),
I think we should not allow subset matching.

Name and all labels must be matched.

My feeling is exactly the same as yours. Some people can be helped by subset matching, but too complicated to handle for us. Thank you for a great opportunity to re-think it and letting me know your cool thought!

**What this PR does / why we need it**: Just removed a TODO comment. We just settled that we don't have to merge events when committing, on this discussion: #1429 (comment) **Which issue(s) this PR fixes**: Fixes #1406 **Does this PR introduce a user-facing change?**:  ```release-note NONE ``` This PR was merged by Kapetanios.

nakabonne · 2021-01-14T09:03:40Z

/hold

pipecd-bot · 2021-01-14T09:09:58Z

Code coverage for golang is 33.87%. This pull request decreases coverage by -0.09%.

File	Function	Base	Head	Diff
pkg/app/piped/apistore/eventstore/store.go	NewStore	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Run	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.sync	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Getter	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.GetLatest	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	eventDefinitionID	`--`	`100.00%`	`+100.00%`
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ListEvents	`0.00%`	`0.00%`	`+0.00%`

nakabonne · 2021-01-14T09:59:44Z

/hold cancel

pipecd-bot · 2021-01-14T10:06:58Z

Code coverage for golang is 33.87%. This pull request decreases coverage by -0.09%.

File	Function	Base	Head	Diff
pkg/app/piped/apistore/eventstore/store.go	NewStore	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Run	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.sync	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Getter	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.GetLatest	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	eventDefinitionID	`--`	`100.00%`	`+100.00%`
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ListEvents	`0.00%`	`0.00%`	`+0.00%`

pkg/app/piped/apistore/eventstore/store.go

nghialv · 2021-01-15T01:37:08Z

pkg/app/piped/apistore/eventstore/store.go

+	// Make the cache up-to-date by traversing events sorted by oldest first.
+	var latestTime int64
+	s.mu.Lock()
+	for _, event := range resp.Events {


resp.Events may contain multiple events for the same key/definition-id, so I think we should build their key/definition-id and filter duplicates before locking the mutex to update the list.

Good point. I initially attempted to do so, but I didn't. That's necessary to keep the time to lock mutex as short as possible. But don't we really have to care about that performance? I guess we should prioritize to keep simplicity than performance at this time, that's why I settled on just sorting by oldest first and traversing all events.

But don't we really have to care about that performance?

Actually, I didn't want to say that we don't really care about the performance.
What I wanted to say before is that the complexity of the concurrency model is not worth the performance it brings.

But calculating the ID and filtering the duplicates before locking can be done easily. So it's worth it.

You're quite right. Adding filtering is not complicated enough to worry about complexity.

nghialv · 2021-01-15T01:39:45Z

pkg/app/piped/apistore/eventstore/store.go

+	s.mu.Lock()
+	for _, event := range resp.Events {
+		id := eventDefinitionID(event.Name, event.Labels)
+		s.latestEventMap[id] = event


We are doing asynchronous operations so this event can be older than the one stored in the map.
Let's check the CreatedAt before overriding.

At the beginning of the loop, it is more likely to happen as you mentioned. But eventually, all Events on the cache will be up-to-date because the list of events is sorted by oldest first. I'd say it's enough but what do you think?

Ah, now I could get what you're most worried about. Okay, let me fix them.

Yes. The list is updated by GetLatest too.

nghialv · 2021-01-15T01:40:12Z

pkg/app/piped/apistore/eventstore/store.go

+	}
+
+	s.mu.Lock()
+	s.latestEventMap[id] = resp.Event


Here, check CreatedAt too.

For this case, I totally agree with you 👍

pkg/app/piped/apistore/eventstore/store.go

nakabonne · 2021-01-15T03:44:35Z

Thank you for your polite review. I've fixed all of them.

pipecd-bot · 2021-01-15T03:44:59Z

Code coverage for golang is 33.83%. This pull request decreases coverage by -0.13%.

File	Function	Base	Head	Diff
pkg/app/piped/apistore/eventstore/store.go	NewStore	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Run	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.sync	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Getter	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.GetLatest	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	makeEventKey	`--`	`100.00%`	`+100.00%`
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ListEvents	`0.00%`	`0.00%`	`+0.00%`

nghialv · 2021-01-15T03:51:37Z

pkg/app/piped/apistore/eventstore/store.go

+	if err != nil {
+		return fmt.Errorf("failed to list events: %w", err)
+	}
+


nit: Return fast if no events.

Right, the previous one could override the milestone to zero. I appreciate you.

nghialv · 2021-01-15T03:52:57Z

pkg/app/piped/apistore/eventstore/store.go

+	}
+
+	// Eliminate events that have duplicated key.
+	var latestTime int64


~~nit: latestTime := resp.Events[len(resp.Events)-1].CreatedAt~~

nghialv · 2021-01-15T03:54:19Z

pkg/app/piped/apistore/eventstore/store.go

+	s.mu.Unlock()
+
+	// Set the latest one within the result as the next time's "from".
+	s.milestone = latestTime + 1


nit: s.milestone := resp.Events[len(resp.Events)-1].CreatedAt + 1

nghialv · 2021-01-15T03:57:38Z

Nice. Thank you.
Looks good.

nakabonne · 2021-01-15T04:28:22Z

Fixed done.

pipecd-bot · 2021-01-15T04:32:43Z

Code coverage for golang is 33.83%. This pull request decreases coverage by -0.13%.

File	Function	Base	Head	Diff
pkg/app/piped/apistore/eventstore/store.go	NewStore	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Run	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.sync	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.Getter	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	store.GetLatest	`--`	`0.00%`	`+0.00%`
pkg/app/piped/apistore/eventstore/store.go	makeEventKey	`--`	`100.00%`	`+100.00%`
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ListEvents	`0.00%`	`0.00%`	`+0.00%`

nghialv · 2021-01-15T04:56:27Z

Nice.
/lgtm

khanhtc1202 · 2021-01-15T05:09:56Z

🚀
/approve

pipecd-bot · 2021-01-15T05:09:59Z

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

Add apistore for caching the latest events

ab31e9a

pipecd-bot added the release-note-none label Jan 14, 2021

pipecd-bot added size/L area/api area/piped labels Jan 14, 2021

pipecd-bot requested review from khanhtc1202 and nghialv January 14, 2021 06:37

nakabonne commented Jan 14, 2021

View reviewed changes

nghialv reviewed Jan 14, 2021

View reviewed changes

nakabonne mentioned this pull request Jan 14, 2021

Remove TODO comment about merging events #1431

Merged

Fix

bed6663

pipecd-bot added the do-not-merge/hold label Jan 14, 2021

Fix comment

024b479

pipecd-bot removed the do-not-merge/hold label Jan 14, 2021

nghialv reviewed Jan 15, 2021

View reviewed changes

Fix

e68fbe3

nghialv reviewed Jan 15, 2021

View reviewed changes

Fix

bef0082

pipecd-bot added the lgtm label Jan 15, 2021

pipecd-bot added the approved label Jan 15, 2021

pipecd-bot merged commit 4814c5d into master Jan 15, 2021

pipecd-bot deleted the eventstore branch January 15, 2021 05:16

pipecd-bot mentioned this pull request Jan 25, 2021

Update feature status #1474

Merged

		// TODO: Use UTC and let control-plane convert it into the control-plane's Local time
		// Unexpected behavior can be happened if Piped uses different timezone from the control-plane.

Add apistore for caching the latest events #1429

Add apistore for caching the latest events #1429

Uh oh!

Conversation

nakabonne commented Jan 14, 2021

Uh oh!

pipecd-bot commented Jan 14, 2021

1. Use UTC and let control-plane convert it into the control-plane's Local time

This was created by todo plugin since "TODO:" was found in ab31e9a when #1429 was merged. cc: @nakabonne.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jan 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This was created by todo plugin since "TODO:" was found in `ab31e9a` when #1429 was merged. cc: @nakabonne.

nghialv Jan 14, 2021 •

edited

Loading

nakabonne Jan 14, 2021 •

edited

Loading

nghialv Jan 14, 2021 •

edited

Loading

nghialv Jan 14, 2021 •

edited

Loading

nghialv Jan 14, 2021 •

edited

Loading