[tests] Add dbnode capable of being started in-process. #3759

nbroyles · 2021-09-19T21:41:25Z

What this PR does / why we need it:

This commit adds an implementation of resources.Node that
is capable of being started in-process as opposed to
being started via a Docker container.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

Does this PR require updating code package or user-facing documentation?:

nbroyles · 2021-09-19T21:53:29Z

src/integration/resources/inprocess/dbnode.go

+	return nil
+}
+
+func updateDBNodePorts(cfg config.Configuration) (config.Configuration, error) {


Note, this is not the complete list of addresses we could watch for based on the config, but one could see how we would extend this to add support

nbroyles · 2021-09-19T21:53:57Z

src/integration/resources/inprocess/dbnode.go

+	return cfg, nil
+}
+
+func updateDBNodeFilepaths(cfg config.Configuration) (config.Configuration, []string, error) {


Ditto for filepaths

nbroyles · 2021-09-19T21:55:09Z

src/integration/resources/inprocess/dbnode_test.go

+	}, res.Datapoints[0])
+}
+
+func TestWriteTaggedFetchTaggedRoundtrip(t *testing.T) {


One interesting thing you can see from this test is that, while node setup is pretty painless, reading and writing could be more frictionless. Adding a layer of helpers after we reach dtest parity could be really nice here.

nbroyles · 2021-09-19T21:56:24Z

src/integration/resources/inprocess/dbnode_test.go

+	host, err := dbnode.HostDetails(9000)
+	require.NoError(t, err)
+
+	_, err = coord.CreateDatabase(admin.DatabaseCreateRequest{


Would also be nice to simplify database creation as well after we get the foundational stuff in place.

codecov · 2021-09-19T22:22:25Z

Codecov Report

Merging #3759 (3118bef) into master (914b23f) will decrease coverage by 0.0%.
The diff coverage is n/a.

❗ Current head 3118bef differs from pull request most recent head 2b2ce9d. Consider uploading reports for the commit 2b2ce9d to get more accurate results

@@           Coverage Diff            @@
##           master   #3759     +/-   ##
========================================
- Coverage    56.9%   56.8%   -0.1%     
========================================
  Files         552     552             
  Lines       62950   62950             
========================================
- Hits        35835   35804     -31     
- Misses      23927   23947     +20     
- Partials     3188    3199     +11

Flag	Coverage Δ
aggregator	`63.3% <ø> (-0.1%)`	⬇️
cluster	`∅ <ø> (∅)`
collector	`58.4% <ø> (ø)`
dbnode	`60.5% <ø> (-0.1%)`	⬇️
m3em	`46.4% <ø> (ø)`
metrics	`19.7% <ø> (ø)`
msg	`74.3% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 914b23f...2b2ce9d. Read the comment docs.

This commit adds an implementation of resources.Node that is capable of being started in-process as opposed to being started via a Docker container.

jaredjenkins · 2021-09-20T19:50:07Z

src/integration/resources/inprocess/coordinator.go

@@ -166,6 +166,34 @@ func NewCoordinator(cfg config.Configuration, opts CoordinatorOptions) (resource
 	return coord, nil
 }

+// NewEmbeddedCoordinator creates a coordinator from one embedded within an existing
+// db node. This method expects that the DB node has already been started before


How is this expectation guaranteed? Does it error? Should it error if it doesn't?

Lazily at the moment. In other words, coordinator API calls will fail since the http server isn't up to handle them. We can validate that here, but that'll take some rework. Will address in a follow up

yeah failing fast is preferrable.

src/integration/resources/inprocess/dbnode.go

src/integration/resources/inprocess/dbnode_test.go

jaredjenkins · 2021-09-20T20:00:24Z

src/integration/resources/inprocess/dbnode.go

+
+	for i := 0; i < d.cfg.Components(); i++ {
+		select {
+		case <-d.shutdownCh:


What exactly are we looping over here? And what are pulling from the shutdownCh?

A dbnode can be configured to run as just a dbnode or a dbnode and an embedded coordinator. Therefore, we need to receive shutdown signals from however many components have been started.

Yeah I am just confused by what we're looping over here? We don't seem to grab anything from the components?

We're waiting on N many shutdown notifications before we proceed where N is equal to the number of components we started up, which is what d.cfg.Components() returns. Each shutdown notification corresponds to a single component telling us they have finished.

Got it. So I think it would just make sense to make this a method for the server interface...

server.ShutdownComponents(waitTime time.Duration) error

with each component implementing an actual Shutdown() method

errors := make(chan error) for _, c : range components { c := c go func() { // capture the error here errors <- c.Shutdown() } () } // Wait for shutdowns, with appropriate the timeout and error catching.

The two channel setup is just feels awkward and hard to read.

Yeah, I think moving this to the server package is a good call. Though, fwiw, there are still going to be two channels -- one to interrupt the components and one to get notified when the components are done shutting down. Will try to make this as friendly as possible

jaredjenkins

approved, but yeah I don't love the server + component interactions.

* master: [dbnode,query] Allow blocking operations on main thread to be interrupted (#3764) [coordinator] Add support for prometheus remote write compatible backend (#3742) [tests] Add dbnode capable of being started in-process. (#3759)

nbroyles commented Sep 19, 2021

View reviewed changes

nbroyles force-pushed the nb/in-process-coord branch 2 times, most recently from e75f283 to 5251677 Compare September 19, 2021 22:03

nbroyles force-pushed the nb/in-process-dbnode branch from 1e56add to 7d7e51f Compare September 19, 2021 22:04

nbroyles force-pushed the nb/in-process-coord branch from 5251677 to b3ac27f Compare September 20, 2021 16:08

nbroyles force-pushed the nb/in-process-dbnode branch from 7d7e51f to b3877b0 Compare September 20, 2021 16:08

nbroyles force-pushed the nb/in-process-coord branch 2 times, most recently from e8a41e4 to 8c7ebab Compare September 20, 2021 18:31

nbroyles force-pushed the nb/in-process-dbnode branch 2 times, most recently from b12a4d6 to 0a6960b Compare September 20, 2021 18:47

Base automatically changed from nb/in-process-coord to master September 20, 2021 19:07

[tests] Add dbnode capable of being started in-process.

5abce0e

This commit adds an implementation of resources.Node that is capable of being started in-process as opposed to being started via a Docker container.

nbroyles force-pushed the nb/in-process-dbnode branch from 0a6960b to 5abce0e Compare September 20, 2021 19:40

jaredjenkins self-requested a review September 20, 2021 19:48

jaredjenkins reviewed Sep 20, 2021

View reviewed changes

src/integration/resources/inprocess/dbnode.go Show resolved Hide resolved

jaredjenkins reviewed Sep 20, 2021

View reviewed changes

nbroyles and others added 2 commits September 20, 2021 16:27

Add comments

3118bef

Merge branch 'master' into nb/in-process-dbnode

2b2ce9d

jaredjenkins self-requested a review September 21, 2021 15:00

jaredjenkins approved these changes Sep 21, 2021

View reviewed changes

nbroyles merged commit 9fea52c into master Sep 21, 2021

nbroyles deleted the nb/in-process-dbnode branch September 21, 2021 15:19

nbroyles mentioned this pull request Sep 24, 2021

[tests] Improved embedded coordinator support #3779

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tests] Add dbnode capable of being started in-process. #3759

[tests] Add dbnode capable of being started in-process. #3759

nbroyles commented Sep 19, 2021

nbroyles Sep 19, 2021

nbroyles Sep 19, 2021

nbroyles Sep 19, 2021 •

edited

Loading

nbroyles Sep 19, 2021

codecov bot commented Sep 19, 2021 •

edited

Loading

jaredjenkins Sep 20, 2021

nbroyles Sep 20, 2021

jaredjenkins Sep 21, 2021

jaredjenkins Sep 20, 2021

nbroyles Sep 20, 2021 •

edited

Loading

jaredjenkins Sep 20, 2021

nbroyles Sep 20, 2021 •

edited

Loading

jaredjenkins Sep 21, 2021

nbroyles Sep 21, 2021

jaredjenkins left a comment

[tests] Add dbnode capable of being started in-process. #3759

[tests] Add dbnode capable of being started in-process. #3759

Conversation

nbroyles commented Sep 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbroyles Sep 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2021 • edited Loading

Codecov Report

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

nbroyles Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbroyles Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredjenkins left a comment

Choose a reason for hiding this comment

nbroyles Sep 19, 2021 •

edited

Loading

codecov bot commented Sep 19, 2021 •

edited

Loading

nbroyles Sep 20, 2021 •

edited

Loading

nbroyles Sep 20, 2021 •

edited

Loading