feat: Poller implementation periodically fetches workspace configs #2

fxenik · 2023-06-29T15:41:15Z

Description of the change

Implements background fetching of workspace configs. ControlPlane SDK now maintains a local workspace config cache, which is periodically updated by a background goroutine. More specifically:

Poller implementation

Starts a background goroutine that polls for workspace configs using a Client dependency.
modelv2.WorkspaceConfigs now exposes an UpdatedAt function that returns the maximum updated at timestamp of included configs.
Poller leverages this information to only fetch the full configuration on start and updates on every subsequent call.
Poller delegates processing of workspace configs to a WorkspaceConfigHandler function.

WorkspaceConfigCache

Provides Get & Set functions to read/write workspace configs.
Supports subscribers that get notified when configs are updated (through Set).
Get returns a copy of the workspace configs in order to avoid race conditions while subscribers read the result.
Set merges workspace config with previous cache contents, using the following rules:
- merges source and destination definitions. Missing definitions in input have no effect, the rest are replaced.
- missing workspaces are removed.
- workspaces with nil configuration are ignored.
- the rest of workspaces are being updated.
Set produces a notification to all Subscriber channels. Channel readers are guaranteed to get the latest configuration indirectly by using ControlPlane.GetWorkspaceConfigs().

ControlPlane

additional WithPollingInterval to control Poller's interval. If set to 0, polling is disabled.
GetWorkspaceConfigs now fetches config from cache if polling is enabled. If not, it makes an http call using Client.
supports config updates through Subscribe.

Note Poller and WorkspaceConfigCache have been implemented in different commits for easier review.

Notion link

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Fix #1

Checklists

Development

Lint rules pass locally
The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development

Code review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
"Ready for review" label attached to the PR and reviewers mentioned in a comment
Changes have been reviewed by at least one other engineer
Issue from task tracker has a link to this pull request

* Split workspace config model to multiple files * Explicitly name model version in package * Renamed SDK to Control Plane * Refactored sample app for best practices * Used url.URL to configure base URL for better error handling

lvrach · 2024-01-02T18:06:45Z

internal/poller/poller.go

+
+// Start starts the poller goroutine. It will poll for new workspace configs every interval.
+// It will stop polling when the context is cancelled.
+func (p *Poller) Start(ctx context.Context) {


[minor] Although we have a way to stop this goroutine, there is no way to know when it got terminated or not. This could be an issue in some cases.

I would suggest on of the following:

Make Start a blocking method, called Run(ctx context.Context)

Introduce a background context, remove ctx from params and add a Stop method, that blocks until the go routine returns

lvrach · 2024-01-02T18:08:50Z

internal/poller/poller.go

+	}
+
+	if err := p.handler(response); err != nil {
+		return fmt.Errorf("failed to handle workspace configs: %w", err)


[minor] To make the final error message more readable avoid including words like error or failed when wrapping and returning messages, as they will be redundant when wrapped multiple times.

Suggested change

return fmt.Errorf("failed to handle workspace configs: %w", err)

return fmt.Errorf("handling workspace configs: %w", err)

lvrach · 2024-01-02T18:09:12Z

internal/poller/poller.go

+		p.log.Debug("polling for workspace configs")
+		wcs, err := p.client.GetWorkspaceConfigs(ctx)
+		if err != nil {
+			return fmt.Errorf("failed to get workspace configs: %w", err)


[minor]

Suggested change

return fmt.Errorf("failed to get workspace configs: %w", err)

return fmt.Errorf("get workspace configs: %w", err)

lvrach · 2024-01-02T18:12:09Z

internal/cache/cache.go

+	}
+
+	// remove deleted workspace configs (missing ids)
+	currentIds := make([]string, 0, len(c.configs.Workspaces))


[nit-pick] idiomatic way in go

Suggested change

currentIds := make([]string, 0, len(c.configs.Workspaces))

currentIDs := make([]string, 0, len(c.configs.Workspaces))

lvrach · 2024-01-02T18:14:08Z

internal/cache/cache.go

+		return nil
+	}
+
+	return copyConfigs(c.configs)


We should consider the memory usage vs safety tread-offs of coping configs here.

lvrach · 2024-01-02T18:17:21Z

internal/poller/poller_test.go

We should consider writing an integration level test, that would combine multiple components, using httptest instead of mocks

achettyiitr · 2024-01-03T17:52:11Z

internal/cache/cache.go

+	c.updateLock.Lock()
+	defer c.updateLock.Unlock()


We can use read locks here, since we are just reading from the configs.

Suggested change

c.updateLock.Lock()

defer c.updateLock.Unlock()

c.updateLock.RLock()

defer c.updateLock.RUnlock()

achettyiitr · 2024-01-03T17:54:21Z

internal/cache/cache.go

+	c.subscriberLock.Lock()
+	defer c.subscriberLock.Unlock()


We can use read locks here, since we are just reading from the slice.

Suggested change

c.subscriberLock.Lock()

defer c.subscriberLock.Unlock()

c.subscriberLock.RLock()

defer c.subscriberLock.RUnlock()

achettyiitr · 2024-01-03T18:02:59Z

internal/cache/cache.go

+	for k, v := range c.SourceDefinitions {
+		wc.SourceDefinitions[k] = v
+	}
+
+	for k, v := range c.DestinationDefinitions {
+		wc.DestinationDefinitions[k] = v
+	}
+
+	for k, v := range c.Workspaces {
+		wc.Workspaces[k] = v
+	}


Suggested change

for k, v := range c.SourceDefinitions {

wc.SourceDefinitions[k] = v

}

for k, v := range c.DestinationDefinitions {

wc.DestinationDefinitions[k] = v

}

for k, v := range c.Workspaces {

wc.Workspaces[k] = v

}

maps.Copy(wc.SourceDefinitions, c.SourceDefinitions)

maps.Copy(wc.DestinationDefinitions, c.DestinationDefinitions)

maps.Copy(wc.Workspaces, c.Workspaces)

achettyiitr · 2024-01-03T18:04:57Z

internal/cache/cache.go

+	for id, config := range configs.SourceDefinitions {
+		c.configs.SourceDefinitions[id] = config
+	}
+
+	for id, config := range configs.DestinationDefinitions {
+		c.configs.DestinationDefinitions[id] = config
+	}


Suggested change

for id, config := range configs.SourceDefinitions {

c.configs.SourceDefinitions[id] = config

}

for id, config := range configs.DestinationDefinitions {

c.configs.DestinationDefinitions[id] = config

}

maps.Copy(c.configs.SourceDefinitions, configs.SourceDefinitions)

maps.Copy(c.configs.DestinationDefinitions, configs.DestinationDefinitions)

achettyiitr · 2024-01-03T18:19:22Z

internal/cache/cache.go

+	defer c.subscriberLock.Unlock()
+
+	subscriber := &Subscriber{
+		notifications: make(chan notifications.WorkspaceConfigNotification),


Should a buffered channel be more appropriate here? If the consumer of the channel is blocked for some reason, it is going to block all the consumers.

fxenik force-pushed the initial-setup branch 2 times, most recently from 8013cf8 to 17465b6 Compare June 29, 2023 15:59

fxenik force-pushed the poller branch 3 times, most recently from 87f77f5 to 8fa44bf Compare June 29, 2023 18:56

fxenik added 5 commits August 1, 2023 13:16

feat: initial client setup

0b4e79d

chore: cleanup PHONY targets in Makefile

7b4825b

chore: address several review comments

50a01ae

* Split workspace config model to multiple files * Explicitly name model version in package * Renamed SDK to Control Plane * Refactored sample app for best practices * Used url.URL to configure base URL for better error handling

chore: address several review comments

7f259d8

* Split workspace config model to multiple files * Explicitly name model version in package * Renamed SDK to Control Plane * Refactored sample app for best practices * Used url.URL to configure base URL for better error handling

feat: Poller implementation periodically fetches workspace configs

d319418

fxenik force-pushed the poller branch from 8fa44bf to 9546bcd Compare August 1, 2023 10:17

fxenik changed the base branch from initial-setup to main August 1, 2023 10:17

feat: WorkspaceConfigCache with support for update subscriptions

a700f7c

fxenik force-pushed the poller branch from 9546bcd to a700f7c Compare August 1, 2023 10:20

fxenik marked this pull request as ready for review January 2, 2024 13:54

achettyiitr self-requested a review January 2, 2024 13:54

lvrach approved these changes Jan 2, 2024

View reviewed changes

achettyiitr reviewed Jan 3, 2024

View reviewed changes

achettyiitr approved these changes Jan 3, 2024

View reviewed changes

achettyiitr requested a review from fracasula January 3, 2024 18:53

achettyiitr merged commit 6a9bbf4 into main Jan 4, 2024

github-actions bot mentioned this pull request Jan 5, 2024

chore: release 1.0.0 #5

Closed

github-actions bot mentioned this pull request Jan 16, 2024

chore(main): release 1.0.0 #10

Closed

github-actions bot mentioned this pull request Jan 30, 2024

chore(main): release 1.0.0 #13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Poller implementation periodically fetches workspace configs #2

feat: Poller implementation periodically fetches workspace configs #2

fxenik commented Jun 29, 2023 •

edited

Loading

lvrach Jan 2, 2024

lvrach Jan 2, 2024

lvrach Jan 2, 2024

lvrach Jan 2, 2024

lvrach Jan 2, 2024

lvrach Jan 2, 2024

achettyiitr Jan 3, 2024 •

edited

Loading

achettyiitr Jan 3, 2024

achettyiitr Jan 3, 2024

achettyiitr Jan 3, 2024

achettyiitr Jan 3, 2024 •

edited

Loading

	return fmt.Errorf("failed to handle workspace configs: %w", err)
	return fmt.Errorf("handling workspace configs: %w", err)

	return fmt.Errorf("failed to get workspace configs: %w", err)
	return fmt.Errorf("get workspace configs: %w", err)

	currentIds := make([]string, 0, len(c.configs.Workspaces))
	currentIDs := make([]string, 0, len(c.configs.Workspaces))

feat: Poller implementation periodically fetches workspace configs #2

feat: Poller implementation periodically fetches workspace configs #2

Conversation

fxenik commented Jun 29, 2023 • edited Loading

Description of the change

Type of change

Related issues

Checklists

Development

Code review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achettyiitr Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achettyiitr Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

fxenik commented Jun 29, 2023 •

edited

Loading

achettyiitr Jan 3, 2024 •

edited

Loading

achettyiitr Jan 3, 2024 •

edited

Loading