Druid CI tests #5184

egor-ryashin · 2024-07-03T10:47:11Z

No description provided.

begelundmuller · 2024-07-10T12:41:41Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

+	// non-retryable HTTP errors
+	tooManyRedirects = regexp.MustCompile(`stopped after \d+ redirects\z`)
+	invalidProtocol  = regexp.MustCompile(`unsupported protocol scheme`)
+	TLSCert          = regexp.MustCompile(`certificate is not trusted`)
+	// retryable Druid errors
+	coordinatorDown = regexp.MustCompile("A leader node could not be found for")
+	brokerDown      = regexp.MustCompile("There are no available brokers")
+	noObject        = regexp.MustCompile("Object '.*' not found")


Consider prefixing with errPattern or something like that

begelundmuller · 2024-07-10T12:42:38Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

+type coordinatorHTTPCheck struct {
+	c *sqlConnection
+}
+
+var _ retrier.AdditionalTest = &coordinatorHTTPCheck{}
+
+// if Coordinator is down for a transient reason it's not a hard failure
+func (chc *coordinatorHTTPCheck) IsHardFailure(ctx context.Context) (bool, error) {


nit: Move to the bottom of the file (as per the style guide, types should be declared in rough call order)

coordinatorHTTPCheck is used right below

begelundmuller · 2024-07-10T12:45:48Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

 	req.Header.Add("Content-Type", "application/json")
-	resp, err := c.client.Do(req)
+	resp, err := chc.c.client.Do(req)
 	if err != nil {
-		return nil, err
+		return false, err
+	}
+
+	switch resp.StatusCode {
+	case http.StatusTooManyRequests:
+		return false, nil
+	case http.StatusUnauthorized, http.StatusForbidden:
+		return true, fmt.Errorf("Unauthorized request")


These cases will return without closing the body

begelundmuller · 2024-07-10T12:49:46Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

-		resp.Body.Close()
-	})
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.dsn, bodyReader)
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, chc.c.dsn, bodyReader)


Would it be possible to extract the actual request and error handling into a single generic function and use structs for the various request/response types? It might also help ensure consistent error handling everywhere. Like:

req := &queryRequest{...} resp := &segmentsResult{} err := chc.c.request(ctx, req, resp)

let's do that in a separate PR

begelundmuller · 2024-07-10T12:54:32Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

+		switch resp.StatusCode {
+		case http.StatusTooManyRequests:
+			return nil, retrier.Retry, fmt.Errorf("Too many requests")
+		case http.StatusUnauthorized, http.StatusForbidden:
+			return nil, retrier.Fail, fmt.Errorf("Unauthorized request")
+		}


The response body is not closed in these error conditions. Why not use defer?

I've added defer in the other request. Here the body should go to a handler that will parse it outside of the function so that we can support drivers.Rows.Next() semantics.

begelundmuller · 2024-07-10T12:55:28Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

+			transformers := make([]func(any) (any, error), len(columns))
+			for i, c := range types {
+				transformers[i] = identityTransformer
+				switch c {
+				case "TINYINT":


This switch statement is pretty long/deep. Could it be pulled into a separate function?

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

begelundmuller · 2024-07-10T12:57:16Z

runtime/drivers/druid/olap.go

 func (i informationSchema) Lookup(ctx context.Context, db, schema, name string) (*drivers.Table, error) {
-	q := `
+	// ensure Coordinator is ready
+	q := "SELECT * FROM sys.segments LIMIT 1"
+	rows, err := i.c.db.QueryxContext(ctx, q, name)
+	if err != nil {
+		return nil, err
+	}
+	rows.Close()


It should avoid running a separate query to check for readiness on every single query. Also, isn't this already handled in the retrier in the QueryContext?

It's only on Lookup query and it's negligible compared to the whole amount of other requests.
The issues is that the request

SELECT T.TABLE_SCHEMA AS SCHEMA, T.TABLE_NAME AS NAME, T.TABLE_TYPE AS TABLE_TYPE, C.COLUMN_NAME AS COLUMN_NAME, C.DATA_TYPE AS COLUMN_TYPE, C.IS_NULLABLE = 'YES' AS IS_NULLABLE FROM INFORMATION_SCHEMA.TABLES T JOIN INFORMATION_SCHEMA.COLUMNS C ON T.TABLE_SCHEMA = C.TABLE_SCHEMA AND T.TABLE_NAME = C.TABLE_NAME WHERE T.TABLE_SCHEMA = 'druid' AND T.TABLE_NAME = ? ORDER BY SCHEMA, NAME, TABLE_TYPE, C.ORDINAL_POSITION

returns false-negative if the Coordinator is being restarted. Retrier is a more abstract class and it doesn't check if SQL tries to retreive dynamic schema and the will be no error from Druid Router (because if dynamic schema is empty - it's considered OK by the cluster).

added a description.

begelundmuller · 2024-07-10T12:58:54Z

runtime/queries/metricsview_aggregation_test.go

+	if os.Getenv("METRICS_CREDS") == "" {
+		t.Skip("skipping the test without the test instance")
+	}


It should avoid hard-coding this env var in many places through the tests. Consider checking it once in e.g. NewInstanceForDruidProject and calling t.Skip if it's not set there.

begelundmuller · 2024-07-10T13:03:50Z

runtime/testruntime/testruntime.go

+	var err error
+	creds := os.Getenv("METRICS_CREDS")
+	address := os.Getenv("METRICS_ADDRESS")
+	data, err := base64.StdEncoding.DecodeString(creds)
+	if err != nil {
+		return nil, "", err
+	}


Can we replace this with a single env var RILL_RUNTIME_DRUID_TEST_DSN? And save it to our shared .env file so everyone on the team is able to run it (see rill devtool dotenv -h)

begelundmuller · 2024-07-16T11:22:16Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

+type coordinatorHTTPCheck struct {
+	c *sqlConnection
+}
+
+var _ retrier.AdditionalTest = &coordinatorHTTPCheck{}


Move to later in the file – style guide says to sort functions in call order with exported functionality first (i.e. QueryContext first)

It didn't complain when CI tests were run. Shouldn't we skip it - it's looks good only from one side but it contradicts the rule - place dependent parts closer together to avoid scrolling back and forth.

It's not a lint rule, but described in the style guide here: https://github.com/uber-go/guide/blob/master/style.md#function-grouping-and-ordering.

I think it's a good rule to sort declarations by call order – it doesn't feel intuitive to look at a Druid driver and the first 100+ lines of code are internal network retrying logic.

But otherwise you'll see the struct you don't know and have to scroll to figure out what it is.

Moreover I believe if the auto-formatter cannot render a style rule by itself then it's a maintenance burden - it consumes your focus away from thinking about logic.

begelundmuller · 2024-07-16T11:24:09Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

+		resp, err := c.client.Do(req)
+		if err != nil {
+			// nolint:errorlint // there's no wrapping
+			if v, ok := err.(*url.Error); ok {


We don't know how the library will change in the future. Better to use errors.As which is created for this.

How can I get the message then?

Isn't that what errors.As helps with? Maybe you're confusing errors.Is and errors.As?

I mean error.As returns a bool but then after that I have to get the error message.
I agree that the library implementation can change but it will be incompatible changes for all clients (which is rare and a bad practice by the library). But anyway, if they change they can wrap it double time and this code fails again. So I should Unwrap recursively - which looks weird and clumsy for error handling.
So for example what can happen if the error is wrapped double time:

type Er1 struct { } func (*Er1) Error() string { return "er1" } type wrapError struct { msg string err error } func (w *wrapError) Error() string { return w.msg } func (w *wrapError) Unwrap() error { return w.err } func ret() error { var e error e = &Er1{} e = &wrapError{ msg: "extra info", err: e, } e = &wrapError{ msg: "superextra info", err: e, } return e } var _ error = &Er1{} func TestError(t *testing.T) { e := ret() e0 := &Er1{} require.True(t, errors.As(e, &e0)) require.Equal(t, "er1", errors.Unwrap(e).Error()) }

expected: "er1" actual : "extra info"

errors.As doesn't just return a bool, it also assigns the error – it handles the recursive unwrapping automatically. Check the docs, I'm pretty sure it's all you need here

begelundmuller · 2024-07-16T11:25:33Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

+			// nolint:errorlint // there's no wrapping
+			if v, ok := err.(*url.Error); ok {
+				if errTooManyRedirects.MatchString(v.Error()) {
+					resp.Body.Close()


This may panic – accessing the body when the request failed.

It's a problem multiple places, I would serialize consider if this can be refactored to use only defer.

Thanks. But defer won't work - see next comment.

Okay, got it. In that case, it's worth considering if things can be re-organized in such a way that resp.Body.Close needs to be manually called in as few cases as possible (to reduce the chance of developer errors, as well as the risk of a buffer leak if a panic occurs)

Let's think about it in another PR.

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

begelundmuller · 2024-07-16T11:30:17Z

runtime/testruntime/testruntime.go

+	var err error
+	b64 := os.Getenv("RILL_RUNTIME_DRUID_TEST_DSN")
+	dsn, err := base64.StdEncoding.DecodeString(b64)
+	if err != nil {
+		return nil, "", err
+	}


Why does it need to be base64 encoded? We have other env variables containing DSNs that are not.

It contains password in format https://username:password@hostname...
Bash has special symbols that the password can contain, to reduce the mental burden remembering which ones I just coded it into base64.

I thought bash/env variables can contain any ASCII string? Just need to escape it or use single quotes to enter it.

Given that all other environment variables in the project do not use base64, I think we should avoid it here as well.

Let's try it.

runtime/testruntime/testruntime.go

begelundmuller

I left some follow-up comments on various previous review comments. Please take a look as some of them require some more changes.

begelundmuller · 2024-07-19T12:53:13Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

+	re := retrier.NewRetrier(6, 2*time.Second, &coordinatorHTTPCheck{
+		c: c,
+	})
+	return re.RunCtx(ctx, func(ctx2 context.Context) (driver.Rows, retrier.Action, error) {


Is it on purpose that most calls use ctx instead of ctx2? What is the implication of using one vs. the other?

begelundmuller · 2024-07-19T12:55:47Z

runtime/drivers/druid/druidsqldriver/druid_api_sql_driver.go

-					return l, nil
+			}
+
+			return nil, retrier.Retry, err


Does it make sense to retry when we don't even get a response back? For example, might just be the DSN that's broken or a local firewall, it's not great to have 126 seconds of retrying just to discover that.

Should we maybe opt for just retrying on the retryable HTTP status codes?

Makes sense.

runtime/testruntime/testruntime.go

begelundmuller · 2024-08-12T11:11:14Z

runtime/testruntime/testruntime.go

@@ -224,11 +230,20 @@ func NewInstanceForProject(t TestingT, name string) (*runtime.Runtime, string) {
 	return rt, inst.ID
 }

-func NewInstanceForDruidProject(t TestingT) (*runtime.Runtime, string) {
+func NewInstanceForDruidProject(t *testing.T) (*runtime.Runtime, string, error) {
+	_, err := os.Stat(".env")


I believe Go tests always evaluate relative to the test file, not the repo root. So you probably need to do something similar to this to get the path of the root .env file:

rill/runtime/testruntime/testruntime.go

Lines 181 to 182 in d66a28f

_, currentFile, _, _ := goruntime.Caller(0)

projectPath := filepath.Join(currentFile, "..", "testdata", name)

Please test and confirm that the approach works

begelundmuller · 2024-08-12T11:11:52Z

runtime/testruntime/testruntime.go

-func NewInstanceForDruidProject(t TestingT) (*runtime.Runtime, string) {
+func NewInstanceForDruidProject(t *testing.T) (*runtime.Runtime, string, error) {
+	_, err := os.Stat(".env")
+	if !errors.Is(err, fs.ErrNotExist) {


There can be other errors than file not existing – it would probably be better to do if err == nil, which is the only way to know that the file actually exists

begelundmuller · 2024-08-12T11:13:23Z

runtime/testruntime/testruntime.go

+func NewInstanceForDruidProject(t *testing.T) (*runtime.Runtime, string, error) {
+	_, err := os.Stat(".env")
+	if !errors.Is(err, fs.ErrNotExist) {
+		require.NoError(t, godotenv.Load())


Related to the comment above – you may need to pass the repo root path here for this to work

egor-ryashin · 2024-08-12T17:53:06Z

Done.

begelundmuller · 2024-08-22T14:45:36Z

runtime/testruntime/testruntime.go

-func NewInstanceForDruidProject(t TestingT) (*runtime.Runtime, string) {
+func NewInstanceForDruidProject(t *testing.T) (*runtime.Runtime, string, error) {
+	_, currentFile, _, _ := goruntime.Caller(0)
+	envPath := filepath.Join(currentFile, "..", "..", ".env")


Won't this resolve to <repo>/runtime/.env, not <repo>/.env? Can you try any changes to confirm?

Druid CI tests

bec2af8

egor-ryashin requested a review from begelundmuller July 3, 2024 10:47

Egor Ryashin added 5 commits July 3, 2024 13:48

Druid CI tests

34b8e4a

Druid CI tests

eaf07e9

Druid CI tests

e19f6a3

Druid CI tests

c269d5d

Druid CI tests

03b3636

egor-ryashin mentioned this pull request Jul 3, 2024

Comparison aggregation unnest and where clause fix #5090

Closed

Druid CI tests

9f3749e

begelundmuller requested changes Jul 10, 2024

View reviewed changes

Egor Ryashin added 8 commits July 10, 2024 23:04

Druid CI tests - body closing

55f44c9

Druid CI tests - body closing

2c9c5bd

Druid CI tests - review

2921d61

Druid CI tests - review

5d506f5

Druid CI tests - review

4064def

Druid CI tests - review

0bbf6c3

Druid CI tests - review

f78881f

Druid CI tests - review

91e163a

egor-ryashin requested a review from begelundmuller July 11, 2024 15:24

Egor Ryashin added 2 commits July 11, 2024 20:34

Merge remote-tracking branch 'origin/main' into druid-ci-tests

7fa433c

Druid CI tests - review

ef03ef9

begelundmuller requested changes Jul 16, 2024

View reviewed changes

Druid CI tests - review

44841d9

egor-ryashin requested a review from begelundmuller July 16, 2024 16:18

begelundmuller reviewed Jul 17, 2024

View reviewed changes

Egor Ryashin added 5 commits July 17, 2024 20:00

Druid CI tests - review - method order

a8e7c46

Druid CI tests - review - base64 env

2f8641a

Druid CI tests - review - base64 env

6519d1b

Druid CI tests - review - base64 env

0adaf6c

Druid CI tests - review - errors.As

229472d

Druid CI tests - review - dsn

6e8d5d2

egor-ryashin requested a review from begelundmuller July 18, 2024 11:35

begelundmuller requested changes Jul 19, 2024

View reviewed changes

Egor Ryashin added 2 commits August 6, 2024 13:06

Druid CI tests - review - retry state

1922f1f

Druid CI tests - review - .env auto-load

bb0e5fb

egor-ryashin requested a review from begelundmuller August 6, 2024 18:59

Egor Ryashin added 4 commits August 6, 2024 22:00

Druid CI tests - review - .env auto-load

530621d

Druid CI tests - review - .env auto-load

8a27beb

Druid CI tests - review - .env auto-load

00d8a2e

Druid CI tests - review - .env auto-load

df9e8ac

begelundmuller requested changes Aug 12, 2024

View reviewed changes

Egor Ryashin added 3 commits August 12, 2024 20:48

conncache - .env

0efa8e1

conncache - .env

c8718cd

conncache - .env

de7f932

egor-ryashin requested a review from begelundmuller August 12, 2024 17:52

egor-ryashin added 2 commits August 20, 2024 12:41

druid ci - style

7116c48

druid ci - env

9244a63

begelundmuller requested changes Aug 22, 2024

View reviewed changes

druid ci - env

435fb72

begelundmuller approved these changes Aug 23, 2024

View reviewed changes

begelundmuller merged commit dc46b12 into main Aug 23, 2024
4 checks passed

begelundmuller deleted the druid-ci-tests branch August 23, 2024 15:48

	_, currentFile, _, _ := goruntime.Caller(0)
	projectPath := filepath.Join(currentFile, "..", "testdata", name)

Druid CI tests #5184

Druid CI tests #5184

Conversation

egor-ryashin commented Jul 3, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-ryashin Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

egor-ryashin Jul 17, 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

begelundmuller Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begelundmuller Jul 18, 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

begelundmuller Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-ryashin Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begelundmuller left a 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

Choose a reason for hiding this comment

egor-ryashin commented Aug 12, 2024

begelundmuller Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-ryashin Jul 17, 2024 •

edited

Loading

egor-ryashin Jul 17, 2024 •

edited

Loading

begelundmuller Jul 17, 2024 •

edited

Loading

begelundmuller Jul 18, 2024 •

edited

Loading

begelundmuller Jul 17, 2024 •

edited

Loading

egor-ryashin Jul 16, 2024 •

edited

Loading

begelundmuller Aug 22, 2024 •

edited

Loading