chore: warehouse backend config refactoring #3602

fracasula · 2023-07-07T15:46:46Z

Description

Small refactoring for the backend config in the warehouse.

Changes:

removed duplicated code
decreased number of global variables
simplified backend config management
increased test coverage
better memory management
rewrite of the warehouse GRPC tests
removed tenant manager from slaves
- this is initialized only once in the warehouse Setup() if isMaster()

TODOs

~~write tests for new backend config~~
run warehouse tests with -race
~~follow up on new TODO comments added in the code~~

Notion Ticket

< Notion Link >

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

…re.whBackendConfig

achettyiitr · 2023-08-03T07:03:42Z

warehouse/warehouse.go

-	var connectionFlags backendconfig.ConnectionFlags
 	for _, wConfig := range config {
-		connectionFlags = wConfig.ConnectionFlags // the last connection flags should be enough, since they are all the same in multi-workspace environments


Since we are already doing this in bcManager, no need do it here.

achettyiitr · 2023-08-03T07:03:48Z

warehouse/warehouse.go

-	if val, ok := connectionFlags.Services["warehouse"]; ok {
-		if UploadAPI.connectionManager != nil {
-			UploadAPI.connectionManager.Apply(connectionFlags.URL, val)
-		}
-	}


Since we are already doing this in bcManager, no need do it here.

achettyiitr · 2023-08-03T07:04:39Z

warehouse/warehouse_test.go

@@ -193,173 +190,3 @@ func TestUploadJob_ProcessingStats(t *testing.T) {
 		})
 	}
 }
-
-func Test_GetNamespace(t *testing.T) {


This is moved to bcManager.

fracasula · 2023-08-07T09:36:48Z

warehouse/backend_config.go

 		bcm.internalControlPlaneClient = cpclient.NewInternalClientWithCache(
-			backendconfig.GetConfigBackendURL(),
+			c.GetString("CONFIG_BACKEND_URL", "https://api.rudderstack.com"),


Why do we keep duplicating the variable name and its default? Isn't GetConfigBackendURL good enough?

I wanted to avoid calling global function for backendconfig was difficult to test.

Right, because you replaced the bc with the tenant manager. Let's keep it this way for now then 👍

fracasula · 2023-08-07T09:39:05Z

warehouse/backend_config.go

+		case data, ok := <-ch:
+			if !ok {
+				return


@achettyiitr nice catch 👍

fracasula · 2023-08-07T09:57:46Z

warehouse/backend_config.go

+
+	namespace, err := s.schema.GetNamespace(ctx, source.ID, destination.ID)
+	if err != nil {
+		s.logger.Errorw("getting namespace",


@achettyiitr we don't have enough context here, right? We should at least log the error.

Good point. Let me make the changes in a separate PR.

fracasula · 2023-08-07T09:59:38Z

warehouse/backend_config_test.go

+	for _, tc := range testcases {
+		tc := tc
+
+		t.Run("should return namespace", func(t *testing.T) {


@achettyiitr How come the test name is always the same? Shall we include a name for the test in the test case itself? Something that would describe the type of test in order to make it self-explanatory would be nice.

fracasula · 2023-08-07T10:03:33Z

warehouse/warehouse.go

@@ -1447,12 +1405,23 @@ func Start(ctx context.Context, app app.App) error {
 		}
 	}()

+	g, ctx := errgroup.WithContext(ctx)


@achettyiitr it feels like we don't need a group here since none of the two routines are returning an error. also, we're overriding the context with the group one which could cause confusion in the code after the group that might be using the context. wdyt?

fracasula · 2023-08-07T10:09:11Z

warehouse/backend_config.go

+	s.warehousesMu.Lock()
+	if len(s.warehouses) > 0 {
+		ch <- s.warehouses
+	}
+	s.warehousesMu.Unlock()


@achettyiitr this version was better, now we're holding the lock for the duration of the whole subscribe. I think we should put this code back.

The reason, we are adding this is because there is a race condition happening if we are not subscribers and we get backed config. The subscriber will not get anything until the next time we get the backend config changes
So, during the subscription also, we are sending the last copy of the data.

fracasula · 2023-08-07T14:02:24Z

warehouse/warehouse.go

+		warehouses = lo.Filter(warehouses, func(warehouse model.Warehouse, _ int) bool {
+			return warehouse.Destination.DestinationDefinition.Name == wh.destType
+		})


@achettyiitr we could also do this to save ourselves the next for loop:

warehouses = lo.Filter(warehouses, func(warehouse model.Warehouse, _ int) bool { if warehouse.Destination.DestinationDefinition.Name != wh.destType { return false } if warehouseutils.IDResolutionEnabled() && slices.Contains(warehouseutils.IdentityEnabledWarehouses, wh.destType) { wh.setupIdentityTables(ctx, warehouse) if shouldPopulateHistoricIdentities && warehouse.Destination.Enabled { // non-blocking populate historic identities wh.populateHistoricIdentities(ctx, warehouse) } } return true })

$fracasula$

github-actions bot added the warehouse-team label Jul 7, 2023

$@fracasula$ fracasula force-pushed the chore.whBackendConfig branch from 696a1d7 to 5a0283b Compare July 7, 2023 16:08

github-actions bot added the with tests label Jul 10, 2023

$@fracasula$ fracasula force-pushed the chore.whBackendConfig branch 2 times, most recently from d798c8d to 7bd3c22 Compare July 21, 2023 14:42

fracasula added 25 commits July 26, 2023 09:24

$@fracasula$

chore: typo

b1b0c1b

$@fracasula$

chore: backend-config manager

fbc204f

$@fracasula$

fix: retry

ba0427d

$@fracasula$

fix: api

1deb4ac

$@fracasula$

fix: admin

58f4dfa

$@fracasula$

chore: updating tests

451348c

$@fracasula$

chore: removing unused attributes

87ff175

$@fracasula$

chore: bigquery test fix

f47aaab

$@fracasula$

chore: linting

301e17a

$@fracasula$

chore: moving constructor

0bc99d5

$@fracasula$

chore: refactor wh grpc test

44aeb08

$@fracasula$

chore: removing duplicated code in test

7b95baa

$@fracasula$

chore: cleaning up

cdd1bc8

$@fracasula$

chore: ssh tunneling and logger

b8d36a0

$@fracasula$

chore: removing unused client

8dd4296

$@fracasula$

chore: better routine management

ccf5134

$@fracasula$

chore: rename variable

a5e75a2

$@fracasula$

chore: removing TODOs

7e3d25f

$@fracasula$

chore: more efficient workerIdentifier()

2a30339

$@fracasula$

chore: adding TODOs

c70b229

$@fracasula$

chore: adding info to backendConfigSubscriber

d79319d

$@fracasula$

chore: optimizing concurrent code

39cd70b

$@fracasula$

chore: using logger

0ad24f7

$@fracasula$

chore: initialConfigFetched

ee7b1e1

$@fracasula$

chore: moving var

5c756c7

achettyiitr force-pushed the chore.whBackendConfig branch 6 times, most recently from 6301bf8 to 5bbf8d8 Compare August 1, 2023 15:30

chore: added test case for many subscribers data race

e3a6c6e

achettyiitr force-pushed the chore.whBackendConfig branch from 5bbf8d8 to e3a6c6e Compare August 1, 2023 15:53

Merge branch 'master' of github.com:rudderlabs/rudder-server into cho…

1c8e8fe

…re.whBackendConfig

achettyiitr force-pushed the chore.whBackendConfig branch from 62dee81 to 1c8e8fe Compare August 3, 2023 06:32

achettyiitr reviewed Aug 3, 2023

View reviewed changes

achettyiitr approved these changes Aug 4, 2023

View reviewed changes

cisse21 approved these changes Aug 7, 2023

View reviewed changes

cisse21 merged commit e48c98e into master Aug 7, 2023
37 checks passed

cisse21 deleted the chore.whBackendConfig branch August 7, 2023 09:26

$fracasula$

fracasula commented Aug 7, 2023

View reviewed changes

achettyiitr added a commit that referenced this pull request Aug 7, 2023

chore: addressing review coments from #3602

e031635

achettyiitr mentioned this pull request Aug 7, 2023

chore: addressing review coments from #3602 #3713

Merged

1 task

$fracasula$

fracasula commented Aug 7, 2023

View reviewed changes

achettyiitr added a commit that referenced this pull request Aug 8, 2023

chore: addressing review coments from #3602 (#3713)

141d109

This was referenced Aug 9, 2023

chore: prerelease 1.12.0-rc.1 #3727

Merged

chore: prerelease 1.12.0-rc.2 #3738

Merged

devops-github-rudderstack mentioned this pull request Aug 14, 2023

chore: release 1.12.0 #3747

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: warehouse backend config refactoring #3602

chore: warehouse backend config refactoring #3602

$@fracasula$ fracasula commented Jul 7, 2023 •

edited by achettyiitr

achettyiitr Aug 3, 2023

achettyiitr Aug 3, 2023

achettyiitr Aug 3, 2023

$@fracasula$ fracasula Aug 7, 2023

achettyiitr Aug 7, 2023 •

edited

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

achettyiitr Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

achettyiitr Aug 7, 2023 •

edited

$@fracasula$ fracasula Aug 7, 2023

chore: warehouse backend config refactoring #3602

chore: warehouse backend config refactoring #3602

Conversation

fracasula commented Jul 7, 2023 • edited by achettyiitr

Description

TODOs

Notion Ticket

Security

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 Aug 7, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achettyiitr Aug 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

$@fracasula$ fracasula commented Jul 7, 2023 •

edited by achettyiitr

achettyiitr Aug 7, 2023 •

edited

achettyiitr Aug 7, 2023 •

edited