From e9439e040ac029b30a61cce189c1767528be3fad Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Fri, 20 Nov 2020 15:55:09 +0200 Subject: [PATCH 01/12] tests for response body --- .../dtest/docker/harness/query_api_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/cmd/tools/dtest/docker/harness/query_api_test.go b/src/cmd/tools/dtest/docker/harness/query_api_test.go index 5ef751b2ec..9ec0907219 100644 --- a/src/cmd/tools/dtest/docker/harness/query_api_test.go +++ b/src/cmd/tools/dtest/docker/harness/query_api_test.go @@ -21,6 +21,7 @@ package harness import ( + "encoding/json" "fmt" "strings" "testing" @@ -145,6 +146,24 @@ func verifyResponse(expectedStatus int) resources.ResponseVerifier { return fmt.Errorf("expected json content type, got %v", contentType) } + errorResponse := struct { + Status string `json:"status,omitempty"` + Error string `json:"error,omitempty"` + }{} + + err = json.Unmarshal([]byte(resp), &errorResponse) + if err != nil { + return fmt.Errorf("failed unmarshalling response: %w", err) + } + + if errorResponse.Status != "error" { + return fmt.Errorf("expected body to contain status 'error', got %v", errorResponse.Status) + } + + if errorResponse.Error == "" { + return fmt.Errorf("expected body to contain error message") + } + return nil } } From 7b9667982743bf6251528b4b2832a23f166dc0c6 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Fri, 20 Nov 2020 15:58:24 +0200 Subject: [PATCH 02/12] add "status" field to /m3query error response --- src/x/net/http/errors.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index a0fbcde08d..f4ed7501bd 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -68,7 +68,8 @@ func (e errorWithCode) Code() int { // ErrorResponse is a generic response for an HTTP error. type ErrorResponse struct { - Error string `json:"error"` + Status string `json:"status"` + Error string `json:"error"` } type options struct { @@ -96,7 +97,7 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { if o.response == nil { w.Header().Set(HeaderContentType, ContentTypeJSON) w.WriteHeader(statusCode) - json.NewEncoder(w).Encode(ErrorResponse{Error: err.Error()}) + json.NewEncoder(w).Encode(ErrorResponse{Status: "error", Error: err.Error()}) } else { w.WriteHeader(statusCode) w.Write(o.response) From bfa2a6232bbee5c4f3fae6496f72b6325d76bf43 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Mon, 23 Nov 2020 10:45:43 +0200 Subject: [PATCH 03/12] fix tests --- .../coordinator_noop/test.sh | 2 +- src/query/api/v1/handler/database/create_test.go | 6 ++++-- src/query/api/v1/handler/namespace/add_test.go | 4 +++- src/query/api/v1/handler/namespace/delete_test.go | 4 +++- src/query/api/v1/handler/namespace/schema_test.go | 4 ++-- src/query/api/v1/handler/namespace/update_test.go | 4 +++- src/query/api/v1/handler/placement/add_test.go | 14 +++++++++----- src/query/api/v1/handler/placement/delete_test.go | 6 ++++-- src/query/api/v1/handler/placement/init_test.go | 4 +++- src/query/api/v1/handler/placement/replace_test.go | 6 ++++-- .../v1/handler/prometheus/native/list_tags_test.go | 7 ++----- .../handler/prometheus/remote/tag_values_test.go | 4 ++-- src/query/api/v1/handler/topic/init_test.go | 2 +- src/query/util/logging/log_test.go | 4 ++-- 14 files changed, 43 insertions(+), 28 deletions(-) diff --git a/scripts/docker-integration-tests/coordinator_noop/test.sh b/scripts/docker-integration-tests/coordinator_noop/test.sh index e6e244b11a..9a3a746c4f 100755 --- a/scripts/docker-integration-tests/coordinator_noop/test.sh +++ b/scripts/docker-integration-tests/coordinator_noop/test.sh @@ -43,7 +43,7 @@ if ! curl -vvvsSf localhost:7201/api/v1/services/m3coordinator/placement; then exit 1 fi -QUERY_EXP='{"error":"operation not valid for noop client"}' +QUERY_EXP='{"status":"error","error":"operation not valid for noop client"}' RES=$(curl "localhost:7201/m3query/api/v1/query_range?start=$(date '+%s')&end=$(date '+%s')&step=10&query=foo") if [[ "$RES" != "$QUERY_EXP" ]]; then echo "Expected resp '$QUERY_EXP', GOT '$RES'" diff --git a/src/query/api/v1/handler/database/create_test.go b/src/query/api/v1/handler/database/create_test.go index 35f46b2f43..b074a13cec 100644 --- a/src/query/api/v1/handler/database/create_test.go +++ b/src/query/api/v1/handler/database/create_test.go @@ -1131,7 +1131,8 @@ func TestClusterTypeMissingHostnames(t *testing.T) { assert.Equal(t, xtest.MustPrettyJSONMap(t, xjson.Map{ - "error": "missing required field", + "status": "error", + "error": "missing required field", }, ), xtest.MustPrettyJSONString(t, string(body))) @@ -1166,7 +1167,8 @@ func TestBadType(t *testing.T) { assert.Equal(t, xtest.MustPrettyJSONMap(t, xjson.Map{ - "error": "invalid database type", + "status": "error", + "error": "invalid database type", }, ), xtest.MustPrettyJSONString(t, string(body))) diff --git a/src/query/api/v1/handler/namespace/add_test.go b/src/query/api/v1/handler/namespace/add_test.go index d0aae7c984..60a94c6354 100644 --- a/src/query/api/v1/handler/namespace/add_test.go +++ b/src/query/api/v1/handler/namespace/add_test.go @@ -100,7 +100,9 @@ func TestNamespaceAddHandler(t *testing.T) { body, err := ioutil.ReadAll(resp.Body) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - assert.Equal(t, "{\"error\":\"bad namespace metadata: retention options must be set\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"bad namespace metadata: retention options must be set"}`, + string(body)) // Test good case. Note: there is no way to tell the difference between a boolean // being false and it not being set by a user. diff --git a/src/query/api/v1/handler/namespace/delete_test.go b/src/query/api/v1/handler/namespace/delete_test.go index 08fd5bb1b0..268b9dcbe2 100644 --- a/src/query/api/v1/handler/namespace/delete_test.go +++ b/src/query/api/v1/handler/namespace/delete_test.go @@ -56,7 +56,9 @@ func TestNamespaceDeleteHandlerNotFound(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) assert.Equal(t, http.StatusNotFound, resp.StatusCode) - assert.Equal(t, "{\"error\":\"unable to find a namespace with specified name\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"unable to find a namespace with specified name"}`, + string(body)) } func TestNamespaceDeleteHandlerDeleteAll(t *testing.T) { diff --git a/src/query/api/v1/handler/namespace/schema_test.go b/src/query/api/v1/handler/namespace/schema_test.go index 80165d2e5d..e5ceaeefd6 100644 --- a/src/query/api/v1/handler/namespace/schema_test.go +++ b/src/query/api/v1/handler/namespace/schema_test.go @@ -158,7 +158,7 @@ func TestSchemaDeploy_KVKeyNotFound(t *testing.T) { body, err := ioutil.ReadAll(resp.Body) assert.NoError(t, err) assert.Equal(t, http.StatusNotFound, resp.StatusCode) - assert.Equal(t, "{\"error\":\"namespace is not found\"}\n", string(body)) + assert.JSONEq(t, `{"status":"error","error":"namespace is not found"}`, string(body)) } func TestSchemaDeploy(t *testing.T) { @@ -250,7 +250,7 @@ func TestSchemaDeploy_NamespaceNotFound(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) assert.Equal(t, http.StatusNotFound, resp.StatusCode) - assert.Equal(t, "{\"error\":\"namespace is not found\"}\n", string(body)) + assert.JSONEq(t, `{"status":"error","error":"namespace is not found"}`, string(body)) } func TestSchemaReset(t *testing.T) { diff --git a/src/query/api/v1/handler/namespace/update_test.go b/src/query/api/v1/handler/namespace/update_test.go index 988c14b1ce..a757f813e6 100644 --- a/src/query/api/v1/handler/namespace/update_test.go +++ b/src/query/api/v1/handler/namespace/update_test.go @@ -107,7 +107,9 @@ func TestNamespaceUpdateHandler(t *testing.T) { body, err := ioutil.ReadAll(resp.Body) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - assert.Equal(t, "{\"error\":\"unable to validate update request: update options cannot be empty\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"unable to validate update request: update options cannot be empty"}`, + string(body)) // Test good case. Note: there is no way to tell the difference between a boolean // being false and it not being set by a user. diff --git a/src/query/api/v1/handler/placement/add_test.go b/src/query/api/v1/handler/placement/add_test.go index 2d66ddc6f7..7526fa4236 100644 --- a/src/query/api/v1/handler/placement/add_test.go +++ b/src/query/api/v1/handler/placement/add_test.go @@ -78,7 +78,9 @@ func TestPlacementAddHandler_Force(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) - assert.Equal(t, "{\"error\":\"no new instances found in the valid zone\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"no new instances found in the valid zone"}`, + string(body)) assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) // Test add success @@ -137,7 +139,9 @@ func TestPlacementAddHandler_SafeErr_NoNewInstance(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) - assert.Equal(t, "{\"error\":\"no new instances found in the valid zone\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"no new instances found in the valid zone"}`, + string(body)) }) } @@ -174,8 +178,8 @@ func TestPlacementAddHandler_SafeErr_NotAllAvailable(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - assert.Equal(t, - `{"error":"instances do not have all shards available: [A, B]"}`+"\n", + assert.JSONEq(t, + `{"status":"error","error":"instances do not have all shards available: [A, B]"}`, string(body)) }) } @@ -243,7 +247,7 @@ func TestPlacementAddHandler_SafeOK(t *testing.T) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) require.Equal(t, http.StatusInternalServerError, resp.StatusCode) - require.Equal(t, `{"error":"test err"}`+"\n", string(body)) + require.JSONEq(t, `{"status":"error","error":"test err"}`, string(body)) w = httptest.NewRecorder() if serviceName == handleroptions.M3AggregatorServiceName { diff --git a/src/query/api/v1/handler/placement/delete_test.go b/src/query/api/v1/handler/placement/delete_test.go index 01402b4514..b8c5027d88 100644 --- a/src/query/api/v1/handler/placement/delete_test.go +++ b/src/query/api/v1/handler/placement/delete_test.go @@ -134,7 +134,7 @@ func TestPlacementDeleteHandler_Force(t *testing.T) { body, err = ioutil.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, http.StatusNotFound, resp.StatusCode) - require.Equal(t, "{\"error\":\"instance not found: nope\"}\n", string(body)) + require.JSONEq(t, `{"status":"error","error":"instance not found: nope"}`, string(body)) }) } @@ -267,7 +267,9 @@ func testDeleteHandlerSafe(t *testing.T, serviceName string) { body, err = ioutil.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, http.StatusBadRequest, resp.StatusCode) - require.Equal(t, `{"error":"instances do not have all shards available: [host2]"}`+"\n", string(body)) + require.JSONEq(t, + `{"status":"error","error":"instances do not have all shards available: [host2]"}`, + string(body)) } // Test OK diff --git a/src/query/api/v1/handler/placement/init_test.go b/src/query/api/v1/handler/placement/init_test.go index 3bec2ce906..eef51e58c5 100644 --- a/src/query/api/v1/handler/placement/init_test.go +++ b/src/query/api/v1/handler/placement/init_test.go @@ -135,7 +135,9 @@ func TestPlacementInitHandler(t *testing.T) { body, err = ioutil.ReadAll(resp.Body) require.NoError(t, err) assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) - assert.Equal(t, "{\"error\":\"unable to build initial placement\"}\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"unable to build initial placement"}`, + string(body)) // Test error response w = httptest.NewRecorder() diff --git a/src/query/api/v1/handler/placement/replace_test.go b/src/query/api/v1/handler/placement/replace_test.go index e45ff85a9d..a4d44ef7a7 100644 --- a/src/query/api/v1/handler/placement/replace_test.go +++ b/src/query/api/v1/handler/placement/replace_test.go @@ -91,7 +91,7 @@ func testPlacementReplaceHandlerForce(t *testing.T, serviceName string) { resp := w.Result() body, _ := ioutil.ReadAll(resp.Body) - assert.Equal(t, `{"error":"test"}`+"\n", string(body)) + assert.JSONEq(t, `{"status":"error","error":"test"}`, string(body)) assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) w = httptest.NewRecorder() @@ -136,7 +136,9 @@ func testPlacementReplaceHandlerSafeErr(t *testing.T, serviceName string) { assert.Equal(t, http.StatusOK, resp.StatusCode) default: assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - assert.Equal(t, `{"error":"instances do not have all shards available: [A, B]"}`+"\n", string(body)) + assert.JSONEq(t, + `{"status":"error","error":"instances do not have all shards available: [A, B]"}`, + string(body)) } } diff --git a/src/query/api/v1/handler/prometheus/native/list_tags_test.go b/src/query/api/v1/handler/prometheus/native/list_tags_test.go index 0ecc6dfa04..f18be297ec 100644 --- a/src/query/api/v1/handler/prometheus/native/list_tags_test.go +++ b/src/query/api/v1/handler/prometheus/native/list_tags_test.go @@ -22,7 +22,6 @@ package native import ( "errors" - "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -179,9 +178,7 @@ func TestListErrorTags(t *testing.T) { r, err := ioutil.ReadAll(body) require.NoError(t, err) - ex := `{"error":"err"}` - // NB: error handler adds a newline to the output. - ex = fmt.Sprintf("%s\n", ex) - require.Equal(t, ex, string(r)) + ex := `{"status":"error","error":"err"}` + require.JSONEq(t, ex, string(r)) } } diff --git a/src/query/api/v1/handler/prometheus/remote/tag_values_test.go b/src/query/api/v1/handler/prometheus/remote/tag_values_test.go index 89a799f39c..120d158fdf 100644 --- a/src/query/api/v1/handler/prometheus/remote/tag_values_test.go +++ b/src/query/api/v1/handler/prometheus/remote/tag_values_test.go @@ -204,6 +204,6 @@ func TestTagValueErrors(t *testing.T) { read, err := ioutil.ReadAll(rr.Body) require.NoError(t, err) - ex := fmt.Sprintf(`{"error":"invalid path with no name present"}%s`, "\n") - assert.Equal(t, ex, string(read)) + ex := `{"status":"error","error":"invalid path with no name present"}` + assert.JSONEq(t, ex, string(read)) } diff --git a/src/query/api/v1/handler/topic/init_test.go b/src/query/api/v1/handler/topic/init_test.go index 78085bec2b..7e33e8d8ad 100644 --- a/src/query/api/v1/handler/topic/init_test.go +++ b/src/query/api/v1/handler/topic/init_test.go @@ -98,5 +98,5 @@ func TestPlacementInitHandler(t *testing.T) { body, err = ioutil.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, http.StatusInternalServerError, resp.StatusCode) - require.Equal(t, "{\"error\":\"init error\"}\n", string(body)) + require.JSONEq(t, `{"status":"error","error":"init error"}`, string(body)) } diff --git a/src/query/util/logging/log_test.go b/src/query/util/logging/log_test.go index 7d0ccca73e..00fab9f63c 100644 --- a/src/query/util/logging/log_test.go +++ b/src/query/util/logging/log_test.go @@ -144,7 +144,7 @@ func TestPanicErrorResponder(t *testing.T) { assert.Equal(t, 500, writer.status) require.Equal(t, 1, len(writer.written)) - assert.Equal(t, "{\"error\":\"caught panic: beef\"}\n", writer.written[0]) + assert.JSONEq(t, `{"status":"error","error":"caught panic: beef"}`, writer.written[0]) assertNoErrorLogs(t, stderr) b, err := ioutil.ReadAll(stdout) @@ -323,7 +323,7 @@ func TestWithResponseTimeAndPanicErrorLoggingFunc(t *testing.T) { assert.Equal(t, 500, writer.status) require.Equal(t, 1, len(writer.written)) - assert.Equal(t, "{\"error\":\"caught panic: err\"}\n", writer.written[0]) + assert.JSONEq(t, `{"status":"error","error":"caught panic: err"}`, writer.written[0]) assertNoErrorLogs(t, stderr) From f956cb42f0f0d4f7f7816053ec7aaa8ab48f6031 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Thu, 17 Dec 2020 02:29:00 +1100 Subject: [PATCH 04/12] [dbnode] Emit consistencyResultError from fetchTaggedResultAccumulator (#3016) --- src/dbnode/client/errors.go | 11 ++++++++--- src/dbnode/client/fetch_tagged_results_accumulator.go | 10 ++++++++-- .../fetch_tagged_results_accumulator_merge_test.go | 4 ++++ 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/dbnode/client/errors.go b/src/dbnode/client/errors.go index 896f1998b4..56ec98e7c2 100644 --- a/src/dbnode/client/errors.go +++ b/src/dbnode/client/errors.go @@ -56,8 +56,13 @@ func IsBadRequestError(err error) bool { // IsConsistencyResultError determines if the error is a consistency result error. func IsConsistencyResultError(err error) bool { - _, ok := err.(consistencyResultErr) - return ok + for err != nil { + if _, ok := err.(consistencyResultErr); ok { //nolint:errorlint + return true + } + err = xerrors.InnerError(err) + } + return false } // NumResponded returns how many nodes responded for a given error @@ -117,8 +122,8 @@ func isHostNotAvailableError(err error) bool { type consistencyResultError interface { error + xerrors.ContainedError - InnerError() error numResponded() int numSuccess() int } diff --git a/src/dbnode/client/fetch_tagged_results_accumulator.go b/src/dbnode/client/fetch_tagged_results_accumulator.go index 32f3b3aa2d..10d87b53cf 100644 --- a/src/dbnode/client/fetch_tagged_results_accumulator.go +++ b/src/dbnode/client/fetch_tagged_results_accumulator.go @@ -205,8 +205,14 @@ func (accum *fetchTaggedResultAccumulator) accumulatedResult( doneAccumulating := true // NB(r): Use new renamed error to keep the underlying error // (invalid/retryable) type. - err := fmt.Errorf("unable to satisfy consistency requirements: shards=%d, err=%v", - accum.numShardsPending, accum.errors) + enqueued := accum.topoMap.HostsLen() + responded := enqueued + consistencyErr := newConsistencyResultError(accum.consistencyLevel, enqueued, responded, + accum.errors) + err := xerrors.Wrapf( + consistencyErr, + "unable to satisfy consistency requirements: shards=%d, err=%v", + accum.numShardsPending, consistencyErr) for i := range accum.errors { if IsBadRequestError(accum.errors[i]) { err = xerrors.NewInvalidParamsError(err) diff --git a/src/dbnode/client/fetch_tagged_results_accumulator_merge_test.go b/src/dbnode/client/fetch_tagged_results_accumulator_merge_test.go index 0981bf9685..92f0ceb138 100644 --- a/src/dbnode/client/fetch_tagged_results_accumulator_merge_test.go +++ b/src/dbnode/client/fetch_tagged_results_accumulator_merge_test.go @@ -343,6 +343,10 @@ func (tm testFetchStateWorkflow) run() fetchTaggedResultAccumulator { } assert.Equal(tm.t, s.expectedDone, done, fmt.Sprintf("i=%d, step=%+v", i, s)) assert.Equal(tm.t, s.expectedErr, err != nil, fmt.Sprintf("i=%d, step=%+v, err=%v", i, s, err)) + if err != nil { + assert.True(tm.t, IsConsistencyResultError(err), + fmt.Sprintf("i=%d, step=%+v, expected consistency result error", i, s)) + } } return accum } From 02eb4500fea93f88062afe4248e315c74778e032 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Thu, 17 Dec 2020 18:32:45 +1100 Subject: [PATCH 05/12] [dbnode] Introduce Resource Exhausted RPC Error flag for query limits (#3017) --- src/dbnode/client/errors.go | 11 ++ src/dbnode/generated/thrift/rpc.thrift | 6 ++ src/dbnode/generated/thrift/rpc/rpc.go | 101 ++++++++++++++++++ .../server/tchannelthrift/cluster/service.go | 15 +-- .../server/tchannelthrift/convert/convert.go | 4 + .../server/tchannelthrift/errors/errors.go | 17 ++- .../tchannelthrift/errors/errors_test.go | 59 ++++++++++ src/dbnode/storage/limits/errors.go | 49 +++++++++ src/dbnode/storage/limits/query_limits.go | 4 +- .../storage/limits/query_limits_test.go | 4 + 10 files changed, 253 insertions(+), 17 deletions(-) create mode 100644 src/dbnode/network/server/tchannelthrift/errors/errors_test.go create mode 100644 src/dbnode/storage/limits/errors.go diff --git a/src/dbnode/client/errors.go b/src/dbnode/client/errors.go index 56ec98e7c2..5b49e501e3 100644 --- a/src/dbnode/client/errors.go +++ b/src/dbnode/client/errors.go @@ -54,6 +54,17 @@ func IsBadRequestError(err error) bool { return false } +// IsResourceExhaustedError determines if the error is a resource exhausted error. +func IsResourceExhaustedError(err error) bool { + for err != nil { + if e, ok := err.(*rpc.Error); ok && tterrors.IsResourceExhaustedErrorFlag(e) { //nolint:errorlint + return true + } + err = xerrors.InnerError(err) + } + return false +} + // IsConsistencyResultError determines if the error is a consistency result error. func IsConsistencyResultError(err error) bool { for err != nil { diff --git a/src/dbnode/generated/thrift/rpc.thrift b/src/dbnode/generated/thrift/rpc.thrift index 112405c525..e83cc7335f 100644 --- a/src/dbnode/generated/thrift/rpc.thrift +++ b/src/dbnode/generated/thrift/rpc.thrift @@ -32,9 +32,15 @@ enum ErrorType { BAD_REQUEST } +enum ErrorFlags { + NONE = 0x00, + RESOURCE_EXHAUSTED = 0x01 +} + exception Error { 1: required ErrorType type = ErrorType.INTERNAL_ERROR 2: required string message + 3: optional i64 flags = 0 } exception WriteBatchRawErrors { diff --git a/src/dbnode/generated/thrift/rpc/rpc.go b/src/dbnode/generated/thrift/rpc/rpc.go index 52d5696af3..678aa3b183 100644 --- a/src/dbnode/generated/thrift/rpc/rpc.go +++ b/src/dbnode/generated/thrift/rpc/rpc.go @@ -162,6 +162,64 @@ func (p *ErrorType) Value() (driver.Value, error) { return int64(*p), nil } +type ErrorFlags int64 + +const ( + ErrorFlags_NONE ErrorFlags = 0 + ErrorFlags_RESOURCE_EXHAUSTED ErrorFlags = 1 +) + +func (p ErrorFlags) String() string { + switch p { + case ErrorFlags_NONE: + return "NONE" + case ErrorFlags_RESOURCE_EXHAUSTED: + return "RESOURCE_EXHAUSTED" + } + return "" +} + +func ErrorFlagsFromString(s string) (ErrorFlags, error) { + switch s { + case "NONE": + return ErrorFlags_NONE, nil + case "RESOURCE_EXHAUSTED": + return ErrorFlags_RESOURCE_EXHAUSTED, nil + } + return ErrorFlags(0), fmt.Errorf("not a valid ErrorFlags string") +} + +func ErrorFlagsPtr(v ErrorFlags) *ErrorFlags { return &v } + +func (p ErrorFlags) MarshalText() ([]byte, error) { + return []byte(p.String()), nil +} + +func (p *ErrorFlags) UnmarshalText(text []byte) error { + q, err := ErrorFlagsFromString(string(text)) + if err != nil { + return err + } + *p = q + return nil +} + +func (p *ErrorFlags) Scan(value interface{}) error { + v, ok := value.(int64) + if !ok { + return errors.New("Scan value is not int64") + } + *p = ErrorFlags(v) + return nil +} + +func (p *ErrorFlags) Value() (driver.Value, error) { + if p == nil { + return nil, nil + } + return int64(*p), nil +} + type AggregateQueryType int64 const ( @@ -223,9 +281,11 @@ func (p *AggregateQueryType) Value() (driver.Value, error) { // Attributes: // - Type // - Message +// - Flags type Error struct { Type ErrorType `thrift:"type,1,required" db:"type" json:"type"` Message string `thrift:"message,2,required" db:"message" json:"message"` + Flags int64 `thrift:"flags,3" db:"flags" json:"flags,omitempty"` } func NewError() *Error { @@ -241,6 +301,16 @@ func (p *Error) GetType() ErrorType { func (p *Error) GetMessage() string { return p.Message } + +var Error_Flags_DEFAULT int64 = 0 + +func (p *Error) GetFlags() int64 { + return p.Flags +} +func (p *Error) IsSetFlags() bool { + return p.Flags != Error_Flags_DEFAULT +} + func (p *Error) Read(iprot thrift.TProtocol) error { if _, err := iprot.ReadStructBegin(); err != nil { return thrift.PrependError(fmt.Sprintf("%T read error: ", p), err) @@ -268,6 +338,10 @@ func (p *Error) Read(iprot thrift.TProtocol) error { return err } issetMessage = true + case 3: + if err := p.ReadField3(iprot); err != nil { + return err + } default: if err := iprot.Skip(fieldTypeId); err != nil { return err @@ -308,6 +382,15 @@ func (p *Error) ReadField2(iprot thrift.TProtocol) error { return nil } +func (p *Error) ReadField3(iprot thrift.TProtocol) error { + if v, err := iprot.ReadI64(); err != nil { + return thrift.PrependError("error reading field 3: ", err) + } else { + p.Flags = v + } + return nil +} + func (p *Error) Write(oprot thrift.TProtocol) error { if err := oprot.WriteStructBegin("Error"); err != nil { return thrift.PrependError(fmt.Sprintf("%T write struct begin error: ", p), err) @@ -319,6 +402,9 @@ func (p *Error) Write(oprot thrift.TProtocol) error { if err := p.writeField2(oprot); err != nil { return err } + if err := p.writeField3(oprot); err != nil { + return err + } } if err := oprot.WriteFieldStop(); err != nil { return thrift.PrependError("write field stop error: ", err) @@ -355,6 +441,21 @@ func (p *Error) writeField2(oprot thrift.TProtocol) (err error) { return err } +func (p *Error) writeField3(oprot thrift.TProtocol) (err error) { + if p.IsSetFlags() { + if err := oprot.WriteFieldBegin("flags", thrift.I64, 3); err != nil { + return thrift.PrependError(fmt.Sprintf("%T write field begin error 3:flags: ", p), err) + } + if err := oprot.WriteI64(int64(p.Flags)); err != nil { + return thrift.PrependError(fmt.Sprintf("%T.flags (3) field write error: ", p), err) + } + if err := oprot.WriteFieldEnd(); err != nil { + return thrift.PrependError(fmt.Sprintf("%T write field end error 3:flags: ", p), err) + } + } + return err +} + func (p *Error) String() string { if p == nil { return "" diff --git a/src/dbnode/network/server/tchannelthrift/cluster/service.go b/src/dbnode/network/server/tchannelthrift/cluster/service.go index 40891ef573..0e2548acdd 100644 --- a/src/dbnode/network/server/tchannelthrift/cluster/service.go +++ b/src/dbnode/network/server/tchannelthrift/cluster/service.go @@ -241,10 +241,7 @@ func (s *service) Fetch(tctx thrift.Context, req *rpc.FetchRequest) (*rpc.FetchR it, err := session.Fetch(nsID, tsID, start, end) if err != nil { - if client.IsBadRequestError(err) { - return nil, tterrors.NewBadRequestError(err) - } - return nil, tterrors.NewInternalError(err) + return nil, convert.ToRPCError(err) } defer it.Close() @@ -340,10 +337,7 @@ func (s *service) Write(tctx thrift.Context, req *rpc.WriteRequest) error { tsID := s.idPool.GetStringID(ctx, req.ID) err = session.Write(nsID, tsID, ts, dp.Value, unit, dp.Annotation) if err != nil { - if client.IsBadRequestError(err) { - return tterrors.NewBadRequestError(err) - } - return tterrors.NewInternalError(err) + return convert.ToRPCError(err) } return nil } @@ -377,10 +371,7 @@ func (s *service) WriteTagged(tctx thrift.Context, req *rpc.WriteTaggedRequest) err = session.WriteTagged(nsID, tsID, ident.NewTagsIterator(tags), ts, dp.Value, unit, dp.Annotation) if err != nil { - if client.IsBadRequestError(err) { - return tterrors.NewBadRequestError(err) - } - return tterrors.NewInternalError(err) + return convert.ToRPCError(err) } return nil } diff --git a/src/dbnode/network/server/tchannelthrift/convert/convert.go b/src/dbnode/network/server/tchannelthrift/convert/convert.go index b2d4ac20f1..ed7b9a7fd0 100644 --- a/src/dbnode/network/server/tchannelthrift/convert/convert.go +++ b/src/dbnode/network/server/tchannelthrift/convert/convert.go @@ -28,6 +28,7 @@ import ( "github.com/m3db/m3/src/dbnode/generated/thrift/rpc" tterrors "github.com/m3db/m3/src/dbnode/network/server/tchannelthrift/errors" "github.com/m3db/m3/src/dbnode/storage/index" + "github.com/m3db/m3/src/dbnode/storage/limits" "github.com/m3db/m3/src/dbnode/x/xio" "github.com/m3db/m3/src/dbnode/x/xpool" "github.com/m3db/m3/src/m3ninx/generated/proto/querypb" @@ -189,6 +190,9 @@ func ToRPCError(err error) *rpc.Error { if err == nil { return nil } + if limits.IsQueryLimitExceededError(err) { + return tterrors.NewResourceExhaustedError(err) + } if xerrors.IsInvalidParams(err) { return tterrors.NewBadRequestError(err) } diff --git a/src/dbnode/network/server/tchannelthrift/errors/errors.go b/src/dbnode/network/server/tchannelthrift/errors/errors.go index 035e3f4fb8..4313aa8357 100644 --- a/src/dbnode/network/server/tchannelthrift/errors/errors.go +++ b/src/dbnode/network/server/tchannelthrift/errors/errors.go @@ -26,10 +26,11 @@ import ( "github.com/m3db/m3/src/dbnode/generated/thrift/rpc" ) -func newError(errType rpc.ErrorType, err error) *rpc.Error { +func newError(errType rpc.ErrorType, err error, flags int64) *rpc.Error { rpcErr := rpc.NewError() rpcErr.Type = errType rpcErr.Message = fmt.Sprintf("%v", err) + rpcErr.Flags = flags return rpcErr } @@ -43,14 +44,24 @@ func IsBadRequestError(err *rpc.Error) bool { return err != nil && err.Type == rpc.ErrorType_BAD_REQUEST } +// IsResourceExhaustedErrorFlag returns whether error has resource exhausted flag. +func IsResourceExhaustedErrorFlag(err *rpc.Error) bool { + return err != nil && err.Flags&int64(rpc.ErrorFlags_RESOURCE_EXHAUSTED) != 0 +} + // NewInternalError creates a new internal error func NewInternalError(err error) *rpc.Error { - return newError(rpc.ErrorType_INTERNAL_ERROR, err) + return newError(rpc.ErrorType_INTERNAL_ERROR, err, int64(rpc.ErrorFlags_NONE)) } // NewBadRequestError creates a new bad request error func NewBadRequestError(err error) *rpc.Error { - return newError(rpc.ErrorType_BAD_REQUEST, err) + return newError(rpc.ErrorType_BAD_REQUEST, err, int64(rpc.ErrorFlags_NONE)) +} + +// NewResourceExhaustedError creates a new resource exhausted error. +func NewResourceExhaustedError(err error) *rpc.Error { + return newError(rpc.ErrorType_BAD_REQUEST, err, int64(rpc.ErrorFlags_RESOURCE_EXHAUSTED)) } // NewWriteBatchRawError creates a new write batch error diff --git a/src/dbnode/network/server/tchannelthrift/errors/errors_test.go b/src/dbnode/network/server/tchannelthrift/errors/errors_test.go new file mode 100644 index 0000000000..6f5f915a4a --- /dev/null +++ b/src/dbnode/network/server/tchannelthrift/errors/errors_test.go @@ -0,0 +1,59 @@ +// Copyright (c) 2020 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package errors + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestErrorsAreRecognized(t *testing.T) { + someError := errors.New("some inner error") + + tests := []struct { + name string + value bool + }{ + { + name: "internal error", + value: IsInternalError(NewInternalError(someError)), + }, + { + name: "bad request error", + value: IsBadRequestError(NewBadRequestError(someError)), + }, + { + name: "resource exhausted error", + value: IsBadRequestError(NewResourceExhaustedError(someError)), + }, + { + name: "resource exhausted flag", + value: IsResourceExhaustedErrorFlag(NewResourceExhaustedError(someError)), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.True(t, tt.value) + }) + } +} diff --git a/src/dbnode/storage/limits/errors.go b/src/dbnode/storage/limits/errors.go new file mode 100644 index 0000000000..a876dd9b7a --- /dev/null +++ b/src/dbnode/storage/limits/errors.go @@ -0,0 +1,49 @@ +// Copyright (c) 2020 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package limits + +import xerrors "github.com/m3db/m3/src/x/errors" + +type queryLimitExceededError struct { + msg string +} + +// NewQueryLimitExceededError creates a query limit exceeded error. +func NewQueryLimitExceededError(msg string) error { + return &queryLimitExceededError{ + msg: msg, + } +} + +func (err *queryLimitExceededError) Error() string { + return err.msg +} + +// IsQueryLimitExceededError returns true if the error is a query limits exceeded error. +func IsQueryLimitExceededError(err error) bool { + for err != nil { + if _, ok := err.(*queryLimitExceededError); ok { //nolint:errorlint + return true + } + err = xerrors.InnerError(err) + } + return false +} diff --git a/src/dbnode/storage/limits/query_limits.go b/src/dbnode/storage/limits/query_limits.go index 49274f83b7..44643d40ec 100644 --- a/src/dbnode/storage/limits/query_limits.go +++ b/src/dbnode/storage/limits/query_limits.go @@ -192,9 +192,9 @@ func (q *lookbackLimit) exceeded() error { func (q *lookbackLimit) checkLimit(recent int64) error { if q.options.Limit > 0 && recent > q.options.Limit { q.metrics.exceeded.Inc(1) - return xerrors.NewInvalidParamsError(fmt.Errorf( + return xerrors.NewInvalidParamsError(NewQueryLimitExceededError(fmt.Sprintf( "query aborted due to limit: name=%s, limit=%d, current=%d, within=%s", - q.name, q.options.Limit, recent, q.options.Lookback)) + q.name, q.options.Limit, recent, q.options.Lookback))) } return nil } diff --git a/src/dbnode/storage/limits/query_limits_test.go b/src/dbnode/storage/limits/query_limits_test.go index 5ea202bbc1..7739812c62 100644 --- a/src/dbnode/storage/limits/query_limits_test.go +++ b/src/dbnode/storage/limits/query_limits_test.go @@ -67,6 +67,7 @@ func TestQueryLimits(t *testing.T) { err = queryLimits.AnyExceeded() require.Error(t, err) require.True(t, xerrors.IsInvalidParams(err)) + require.True(t, IsQueryLimitExceededError(err)) opts = testQueryLimitOptions(docOpts, bytesOpts, instrument.NewOptions()) queryLimits, err = NewQueryLimits(opts) @@ -82,6 +83,7 @@ func TestQueryLimits(t *testing.T) { err = queryLimits.AnyExceeded() require.Error(t, err) require.True(t, xerrors.IsInvalidParams(err)) + require.True(t, IsQueryLimitExceededError(err)) } func TestLookbackLimit(t *testing.T) { @@ -161,6 +163,7 @@ func verifyLimit(t *testing.T, limit *lookbackLimit, inc int, expectedLimit int6 } else { require.Error(t, err) require.True(t, xerrors.IsInvalidParams(err)) + require.True(t, IsQueryLimitExceededError(err)) exceededCount++ } err = limit.exceeded() @@ -169,6 +172,7 @@ func verifyLimit(t *testing.T, limit *lookbackLimit, inc int, expectedLimit int6 } else { require.Error(t, err) require.True(t, xerrors.IsInvalidParams(err)) + require.True(t, IsQueryLimitExceededError(err)) exceededCount++ } return exceededCount From 81292d52abb08c044e83996f3a66d9c6ed96b263 Mon Sep 17 00:00:00 2001 From: Chris Chinchilla Date: Thu, 17 Dec 2020 11:28:58 +0100 Subject: [PATCH 06/12] [DOCS] Fix new API paths and add Docker version (#3018) * Fix new API paths and add Docker version Signed-off-by: ChrisChinchilla * Force add missing shortcode Signed-off-by: ChrisChinchilla --- site/config/_default/config.toml | 4 ++- .../includes/quickstart-common-steps.md | 31 +++++++++++++++---- site/content/quickstart/docker.md | 2 +- site/layouts/shortcodes/docker-version.html | 1 + 4 files changed, 30 insertions(+), 8 deletions(-) create mode 100644 site/layouts/shortcodes/docker-version.html diff --git a/site/config/_default/config.toml b/site/config/_default/config.toml index c1b342376d..9b00108ee0 100644 --- a/site/config/_default/config.toml +++ b/site/config/_default/config.toml @@ -147,11 +147,13 @@ offlineSearch = false [params.api] localCordinator = "http://localhost:7201/" apiEndpoint = "api/v1/" - +# TODO: Can all the below be consolidated? [[params.versions]] version = "1.0" url = "" + [params.releases] + docker = "v1.0.0" # TODO: Do not like doing this really [markup] diff --git a/site/content/includes/quickstart-common-steps.md b/site/content/includes/quickstart-common-steps.md index 4db7a62ceb..ac2c62ccd2 100644 --- a/site/content/includes/quickstart-common-steps.md +++ b/site/content/includes/quickstart-common-steps.md @@ -21,8 +21,6 @@ This quickstart uses the _{{% apiendpoint %}}database/create_ endpoint that crea You can create [placements](/docs/operational_guide/placement_configuration/) and [namespaces](/docs/operational_guide/namespace_configuration/#advanced-hard-way) separately if you need more control over their settings. -The `namespaceName` argument must match the namespace in the `local` section of the `M3Coordinator` YAML configuration. If you [add any namespaces](/docs/operational_guide/namespace_configuration) you also need to add them to the `local` section of `M3Coordinator`'s YAML configuration. - In another terminal, use the following command. {{< tabs name="create_placement_namespace" >}} @@ -125,13 +123,13 @@ Placement initialization can take a minute or two. Once all the shards have the {"level":"info","ts":1598367624.03023,"msg":"bootstrapped"} ``` -You can check on the status by calling the _{{% apiendpoint %}}placement_ endpoint: +You can check on the status by calling the _{{% apiendpoint %}}services/m3db/placement_ endpoint: {{< tabs name="check_placement" >}} {{% tab name="Command" %}} ```shell -curl {{% apiendpoint %}}placement | jq . +curl {{% apiendpoint %}}services/m3db/placement | jq . ``` {{% /tab %}} @@ -190,15 +188,36 @@ curl {{% apiendpoint %}}placement | jq . [Read more about the bootstrapping process](/docs/operational_guide/bootstrapping_crash_recovery/). {{% /notice %}} +### Ready a Namespace + +Once a namespace has finished bootstrapping, you must mark it as ready before receiving traffic by using the _{{% apiendpoint %}}namespace/ready_. + +{{< tabs name="ready_namespaces" >}} +{{% tab name="Command" %}} + +{{% codeinclude file="docs/includes/quickstart/ready-namespace.sh" language="shell" %}} + +{{% /tab %}} +{{% tab name="Output" %}} + +```json +{ +"ready": true +} +``` + +{{% /tab %}} +{{< /tabs >}} + ### View Details of a Namespace -You can also view the attributes of all namespaces by calling the _{{% apiendpoint %}}namespace_ endpoint +You can also view the attributes of all namespaces by calling the _{{% apiendpoint %}}services/m3db/namespace_ endpoint {{< tabs name="check_namespaces" >}} {{% tab name="Command" %}} ```shell -curl {{% apiendpoint %}}namespace | jq . +curl {{% apiendpoint %}}services/m3db/namespace | jq . ``` {{% notice tip %}} diff --git a/site/content/quickstart/docker.md b/site/content/quickstart/docker.md index 9bcb1c5a83..148fb252c9 100644 --- a/site/content/quickstart/docker.md +++ b/site/content/quickstart/docker.md @@ -41,7 +41,7 @@ The command below creates a persistent data directory on the host operating syst {{% tab name="Command" %}} ```shell -docker run -p 7201:7201 -p 7203:7203 --name m3db -v $(pwd)/m3db_data:/var/lib/m3db quay.io/m3db/m3dbnode:latest +docker run -p 7201:7201 -p 7203:7203 --name m3db -v $(pwd)/m3db_data:/var/lib/m3db quay.io/m3db/m3dbnode:{{% docker-version %}} ``` {{% /tab %}} diff --git a/site/layouts/shortcodes/docker-version.html b/site/layouts/shortcodes/docker-version.html new file mode 100644 index 0000000000..2600626c34 --- /dev/null +++ b/site/layouts/shortcodes/docker-version.html @@ -0,0 +1 @@ +{{ .Site.Params.releases.docker }} \ No newline at end of file From a8dce25f3d3027e91ab856068d9125305f50493c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linas=20Med=C5=BEi=C5=ABnas?= Date: Thu, 17 Dec 2020 15:52:22 +0200 Subject: [PATCH 07/12] [dbnode] Add server.StorageOptions to TestSetup (#3023) --- src/dbnode/integration/serve.go | 8 ++++++ src/dbnode/integration/setup.go | 44 +++++++++++++++++---------------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/dbnode/integration/serve.go b/src/dbnode/integration/serve.go index 2b56026f2f..6f229810a3 100644 --- a/src/dbnode/integration/serve.go +++ b/src/dbnode/integration/serve.go @@ -33,6 +33,7 @@ import ( "github.com/m3db/m3/src/dbnode/network/server/tchannelthrift" ttcluster "github.com/m3db/m3/src/dbnode/network/server/tchannelthrift/cluster" ttnode "github.com/m3db/m3/src/dbnode/network/server/tchannelthrift/node" + "github.com/m3db/m3/src/dbnode/server" "github.com/m3db/m3/src/dbnode/sharding" "github.com/m3db/m3/src/dbnode/storage" "github.com/m3db/m3/src/dbnode/topology" @@ -95,6 +96,7 @@ func openAndServe( db storage.Database, client client.Client, opts storage.Options, + serverStorageOpts server.StorageOptions, doneCh <-chan struct{}, ) error { logger := opts.InstrumentOptions().Logger() @@ -106,6 +108,12 @@ func openAndServe( ttopts := tchannelthrift.NewOptions() service := ttnode.NewService(db, ttopts) nodeOpts := ttnode.NewOptions(nil) + if fn := serverStorageOpts.TChanChannelFn; fn != nil { + nodeOpts = nodeOpts.SetTChanChannelFn(fn) + } + if fn := serverStorageOpts.TChanNodeServerFn; fn != nil { + nodeOpts = nodeOpts.SetTChanNodeServerFn(fn) + } nativeNodeClose, err := ttnode.NewServer(service, tchannelNodeAddr, contextPool, nodeOpts).ListenAndServe() if err != nil { return fmt.Errorf("could not open tchannelthrift interface %s: %v", tchannelNodeAddr, err) diff --git a/src/dbnode/integration/setup.go b/src/dbnode/integration/setup.go index 9caa48661d..e4d27478cf 100644 --- a/src/dbnode/integration/setup.go +++ b/src/dbnode/integration/setup.go @@ -42,6 +42,7 @@ import ( "github.com/m3db/m3/src/dbnode/persist/fs/commitlog" "github.com/m3db/m3/src/dbnode/retention" "github.com/m3db/m3/src/dbnode/runtime" + "github.com/m3db/m3/src/dbnode/server" "github.com/m3db/m3/src/dbnode/sharding" "github.com/m3db/m3/src/dbnode/storage" "github.com/m3db/m3/src/dbnode/storage/block" @@ -61,7 +62,7 @@ import ( "github.com/stretchr/testify/require" "github.com/uber-go/tally" - tchannel "github.com/uber/tchannel-go" + "github.com/uber/tchannel-go" "go.uber.org/zap" "go.uber.org/zap/zapcore" ) @@ -80,12 +81,8 @@ var ( testSchemaHistory = prototest.NewSchemaHistory() testSchema = prototest.NewMessageDescriptor(testSchemaHistory) - testSchemaDesc = namespace.GetTestSchemaDescr(testSchema) testProtoMessages = prototest.NewProtoTestMessages(testSchema) - testProtoEqual = func(expect, actual []byte) bool { - return prototest.ProtoEqual(testSchema, expect, actual) - } - testProtoIter = prototest.NewProtoMessageIterator(testProtoMessages) + testProtoIter = prototest.NewProtoMessageIterator(testProtoMessages) ) // nowSetterFn is the function that sets the current time @@ -105,6 +102,7 @@ type testSetup struct { db cluster.Database storageOpts storage.Options + serverStorageOpts server.StorageOptions fsOpts fs.Options blockLeaseManager block.LeaseManager hostID string @@ -133,8 +131,10 @@ type testSetup struct { namespaces []namespace.Metadata // signals - doneCh chan struct{} - closedCh chan struct{} + doneCh chan struct { + } + closedCh chan struct { + } } // TestSetup is a test setup. @@ -157,6 +157,7 @@ type TestSetup interface { FilePathPrefix() string StorageOpts() storage.Options SetStorageOpts(storage.Options) + SetServerStorageOpts(server.StorageOptions) Origin() topology.Host ServerIsBootstrapped() bool StopServer() error @@ -605,6 +606,10 @@ func (ts *testSetup) SetStorageOpts(opts storage.Options) { ts.storageOpts = opts } +func (ts *testSetup) SetServerStorageOpts(opts server.StorageOptions) { + ts.serverStorageOpts = opts +} + func (ts *testSetup) TopologyInitializer() topology.Initializer { return ts.topoInit } @@ -732,7 +737,7 @@ func (ts *testSetup) startServerBase(waitForBootstrap bool) error { if err := openAndServe( ts.httpClusterAddr(), ts.tchannelClusterAddr(), ts.httpNodeAddr(), ts.tchannelNodeAddr(), ts.httpDebugAddr(), - ts.db, ts.m3dbClient, ts.storageOpts, ts.doneCh, + ts.db, ts.m3dbClient, ts.storageOpts, ts.serverStorageOpts, ts.doneCh, ); err != nil { select { case resultCh <- err: @@ -1004,22 +1009,19 @@ func newClients( tchannelNodeAddr string, ) (client.AdminClient, client.AdminClient, error) { var ( - clientOpts = defaultClientOptions(topoInit). - SetClusterConnectTimeout(opts.ClusterConnectionTimeout()). - SetFetchRequestTimeout(opts.FetchRequestTimeout()). - SetWriteConsistencyLevel(opts.WriteConsistencyLevel()). - SetTopologyInitializer(topoInit). - SetUseV2BatchAPIs(true) + clientOpts = defaultClientOptions(topoInit).SetClusterConnectTimeout( + opts.ClusterConnectionTimeout()). + SetFetchRequestTimeout(opts.FetchRequestTimeout()). + SetWriteConsistencyLevel(opts.WriteConsistencyLevel()). + SetTopologyInitializer(topoInit). + SetUseV2BatchAPIs(true) origin = newOrigin(id, tchannelNodeAddr) verificationOrigin = newOrigin(id+"-verification", tchannelNodeAddr) - adminOpts = clientOpts.(client.AdminOptions). - SetOrigin(origin). - SetSchemaRegistry(schemaReg) - verificationAdminOpts = adminOpts. - SetOrigin(verificationOrigin). - SetSchemaRegistry(schemaReg) + adminOpts = clientOpts.(client.AdminOptions).SetOrigin(origin).SetSchemaRegistry(schemaReg) + + verificationAdminOpts = adminOpts.SetOrigin(verificationOrigin).SetSchemaRegistry(schemaReg) ) if opts.ProtoEncoding() { From 7c3a5547d00ece7ef2af6b947dac42840478c8c6 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Fri, 18 Dec 2020 01:11:31 +1100 Subject: [PATCH 08/12] [dbnode] Fix "unable to satisfy consistency requirements" error message (#3025) --- src/dbnode/client/fetch_tagged_results_accumulator.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dbnode/client/fetch_tagged_results_accumulator.go b/src/dbnode/client/fetch_tagged_results_accumulator.go index 10d87b53cf..35256b903e 100644 --- a/src/dbnode/client/fetch_tagged_results_accumulator.go +++ b/src/dbnode/client/fetch_tagged_results_accumulator.go @@ -211,8 +211,8 @@ func (accum *fetchTaggedResultAccumulator) accumulatedResult( accum.errors) err := xerrors.Wrapf( consistencyErr, - "unable to satisfy consistency requirements: shards=%d, err=%v", - accum.numShardsPending, consistencyErr) + "unable to satisfy consistency requirements, shards=%d", + accum.numShardsPending) for i := range accum.errors { if IsBadRequestError(accum.errors[i]) { err = xerrors.NewInvalidParamsError(err) From 0657aef127c6bcc2bca04a413c08b21605dea0be Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Thu, 17 Dec 2020 16:57:04 +0200 Subject: [PATCH 09/12] update OpenAPI spec --- src/query/generated/assets/openapi/assets.go | 20 ++++++++++++++++---- src/query/generated/assets/openapi/spec.yml | 2 ++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/query/generated/assets/openapi/assets.go b/src/query/generated/assets/openapi/assets.go index 2f4409dffe..1f95a84356 100644 --- a/src/query/generated/assets/openapi/assets.go +++ b/src/query/generated/assets/openapi/assets.go @@ -210,6 +210,18 @@ func FSMustString(useLocal bool, name string) string { var _escData = map[string]*_escFile{ + "/asset-gen": { + local: "asset-gen", + size: 238, + modtime: 12345, + compressed: ` +H4sIAAAAAAAC/0zKz0rEMBDH8Xue4rfTnBbSsP45LR5EfAHrTUTWdpIO0hlJIgjiu0sr6M5l4PP9dbv4 +Khrr7NztMNw/vgwPdzf+4JIVCERB/s8p7o+YzAGAJOzwhDDBC56PaDPrFtYbTZvoB2+QxG2fx9lAmZXL +qYlmpGILvNBvrSPCYlOThUGHi8ura0J4L5zkE+S/pOv28Tuu9pb/gRAkqxVGnw3BzqanWrnVPhuBenKT +KbufAAAA//9BiTev7gAAAA== +`, + }, + "/asset-gen.sh": { local: "asset-gen.sh", size: 238, @@ -239,7 +251,7 @@ d8dUBmQZxiF/+uI1I7E8TMF6pCyWxDpY7WPA8pmKZsl6ouawOaOS+Sj+BQAA//8by2IcfAIAAA== "/spec.yml": { local: "openapi/spec.yml", - size: 22779, + size: 22816, modtime: 12345, compressed: ` H4sIAAAAAAAC/+xcX2/bOBJ/96fgqvdw+5A4bXp7gN+cOJsaSNMgyS5wuzhgaXIkc1ciVXKYNF3cdz9Q @@ -275,9 +287,9 @@ FK7RbZ4sol8Ss+4Ap7xuumWQuqSNN3CGMCpMVkXya0sNwl+VbOpjH0EEc2wKGkgeKyGxQZmpzirVmuY3 ewhRM8KSEBcUN8bb/WU/7VRvaaw0tkld973D953B5wlf4fR5n7FsEZ6XdTxB7W4eriXOIEVoIJu0HGHu PMcoqxlMm+K3qJvXVCqzbeF0Onx/axUr2wthy9sJ0kbpzSPiTa+n99Px1fS36fWlt7w4/nU8vRqfXV1k V64uxr8uJCper9gLkW5V1tZWRraJVZpBq7Yl94WMb86L1sRe1eXk2gg3SbtWorZdKj7H7xCtEOiDkME0 -O2nvHrbq5UYlF5zitwimrWv0syMrfwzZZce6kq9MSOXJ8Tabo+tm2kguDhpO4FZlblHDQsVo6OWvsNAa -3KKRftb1VjiUqWzBNxzFNLDa2VIuz9F7gtkHlWLrrGhLq8Rjs4vwJQaGwO+SH351SEt6EHMD+oOyehue -/KD224ZSzjUYs2O/84oN7f5DXP0kZ5uS0PagpuJRaOcDhoKO/wcAAP//ae6OF/tYAAA= +O2nvHrbq5UYlF5zitwimrWv0syMrfwzZwTBXOK1pYoeV0kqZyuPlbXZQ183cklwcNBzTrWrhotCFitHQ +y19hoTW4Rbf9rIuycHJT2advOK9poL6zpVyeyPeExQ8qBeBZ0ZZWicdmF+FLDAyB3yW/DuuQljQq5gb0 +B2X1NmT6Qe23V6WcazBmx6boFbve/Ye4+nHPNiWh7WlOxfPSzqcQBR3/DwAA//8beZKQIFkAAA== `, }, diff --git a/src/query/generated/assets/openapi/spec.yml b/src/query/generated/assets/openapi/spec.yml index b7fe173fb4..7a0194e5d9 100644 --- a/src/query/generated/assets/openapi/spec.yml +++ b/src/query/generated/assets/openapi/spec.yml @@ -856,6 +856,8 @@ definitions: GenericError: type: "object" properties: + status: + type: "string" error: type: "string" DatabaseCreateRequest: From 18f98668eed1e69498eeb8a2c3d8b26379f9b28f Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Thu, 17 Dec 2020 17:00:32 +0200 Subject: [PATCH 10/12] fix lint error --- src/x/net/http/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index 07bb7357ae..082fce1b5d 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -97,7 +97,7 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { if o.response == nil { w.Header().Set(HeaderContentType, ContentTypeJSON) w.WriteHeader(statusCode) - json.NewEncoder(w).Encode(ErrorResponse{Status: "error", Error: err.Error()}) + json.NewEncoder(w).Encode(ErrorResponse{Status: "error", Error: err.Error()}) //nolint:errcheck } else { w.WriteHeader(statusCode) w.Write(o.response) From 5e077b2a670086b8a5d76b377269db20f2228f34 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Thu, 17 Dec 2020 17:24:22 +0200 Subject: [PATCH 11/12] fix assets.go --- src/query/generated/assets/openapi/assets.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/query/generated/assets/openapi/assets.go b/src/query/generated/assets/openapi/assets.go index 1f95a84356..4f5e461d9c 100644 --- a/src/query/generated/assets/openapi/assets.go +++ b/src/query/generated/assets/openapi/assets.go @@ -210,18 +210,6 @@ func FSMustString(useLocal bool, name string) string { var _escData = map[string]*_escFile{ - "/asset-gen": { - local: "asset-gen", - size: 238, - modtime: 12345, - compressed: ` -H4sIAAAAAAAC/0zKz0rEMBDH8Xue4rfTnBbSsP45LR5EfAHrTUTWdpIO0hlJIgjiu0sr6M5l4PP9dbv4 -Khrr7NztMNw/vgwPdzf+4JIVCERB/s8p7o+YzAGAJOzwhDDBC56PaDPrFtYbTZvoB2+QxG2fx9lAmZXL -qYlmpGILvNBvrSPCYlOThUGHi8ura0J4L5zkE+S/pOv28Tuu9pb/gRAkqxVGnw3BzqanWrnVPhuBenKT -KbufAAAA//9BiTev7gAAAA== -`, - }, - "/asset-gen.sh": { local: "asset-gen.sh", size: 238, From a73356b55f834dbb2f28bcc50a4ab1a8d6d8120a Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Fri, 18 Dec 2020 16:55:35 +0200 Subject: [PATCH 12/12] small refactor --- src/query/api/v1/handler/prometheus/native/list_tags_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/query/api/v1/handler/prometheus/native/list_tags_test.go b/src/query/api/v1/handler/prometheus/native/list_tags_test.go index f18be297ec..f33ef27b18 100644 --- a/src/query/api/v1/handler/prometheus/native/list_tags_test.go +++ b/src/query/api/v1/handler/prometheus/native/list_tags_test.go @@ -178,7 +178,6 @@ func TestListErrorTags(t *testing.T) { r, err := ioutil.ReadAll(body) require.NoError(t, err) - ex := `{"status":"error","error":"err"}` - require.JSONEq(t, ex, string(r)) + require.JSONEq(t, `{"status":"error","error":"err"}`, string(r)) } }