add clauses to detect nil Node's in idx.Find #812
Conversation
|
@Dieterbe my build fail on this: |
|
it requires go 1.9 to build. you can also download the builds generated at https://circleci.com/gh/grafana/metrictank/3470#artifacts/containers/0 (navigate to home/circleci/.go_workspace/src/github.com/grafana/metrictank/build/ or build_pkgs at the end) |
|
@Dieterbe , Here is the output: |
previous version didn't catch cases in last loop run and checked startNode twice
they can also be internal server errors also: correctly return internal errors as errors
Dieterbe
force-pushed
the
diagnose-idx-nil-node
branch
from
25dfd47
to
96b47b4
Compare
|
@motyla my previous patch didn't cover all cases. would you mind trying again with the latest patch? (click on the last green checkmark on this page, go to artifacts and you can download from there) |
|
Thanks @Dieterbe , we tried this build yesterday and we're getting messages like: So it seems that the service is not crashing now. |
api/graphite.go
Outdated
| return nil, response.NewError(http.StatusBadRequest, err.Error()) | ||
| err := response.WrapError(err) | ||
| if err.Code() != http.StatusBadRequest { | ||
| tags.Error.Set(span, true) |
There was a problem hiding this comment.
this seems to be the same like the below used tracing.Failure(span) or not? for consistency i'd use either one or the other only, otherwise it's confusing because the reader first needs to figure out that they're the same
idx/memory/memory.go
Outdated
| if pos == 0 { | ||
| //we need to start at the root. | ||
| log.Debug("memory-idx: starting search at the root node") | ||
| startNode = tree.Items[""] | ||
| } else { | ||
| branch := strings.Join(nodes[0:pos], ".") | ||
| branch = strings.Join(nodes[0:pos], ".") |
There was a problem hiding this comment.
couldn't that just be nodes[:pos] without the 0
idx/memory/memory.go
Outdated
| } | ||
| } | ||
|
|
||
| if startNode == nil { |
There was a problem hiding this comment.
when the assignment of startNode on line :894 fails because the searched branch is not present, then ok should be false and the error handler on :895 would return. so the only case how this startNode could be nil is if the assignment in :890 fails or not? so why not just add an if !ok check after line :890 instead
There was a problem hiding this comment.
or is the goal to check if a value of nil was actually stored in tree.Items? then that makes sense, although i think it should be treated as a separate error case from the case where "" does not exist in tree.Items, with the current log statement we wouldn't know if the problem is that tree.Items[""] does not exist or if the problem is that tree.Items[""] refers to a value nil
There was a problem hiding this comment.
!ok means an entry wasn't found in the map (and the returned value is then nil in that case).
however it is also possible there was an entry in the map that is nil (e.g. ok is true, returned value is a nil pointer).
i want to protect against all scenarios. (in fact i have a suspicion there might be a nilpointer in the map)
There was a problem hiding this comment.
I agree with @replay. An index that is empty is not corrupt. We should just return nil, nil after line :890
There was a problem hiding this comment.
i just changed the code to allow for the root node to not exist. and still treat found-but-nil nodes as corruption case. 3f78925
api/response/error.go
Outdated
| @@ -61,6 +62,19 @@ func NewError(code int, err string) *ErrorResp { | |||
| } | |||
| } | |||
|
|
|||
| func NewInternalError(err string) *ErrorResp { | |||
There was a problem hiding this comment.
It doesnt sit well with me to have idx/memory depend on api/response when the idx has a lot of uses that dont use the api.
response.WrapError() will set the status code correctly if the pass in error implements the response.Error interface.
There was a problem hiding this comment.
I agree. ideally none of the internal libraries should import any api libraries. to be more precise: none of the internal libraries should be concerned about http implementation details.
but those internal libraries should be able to denote whether an error is caused by user, or caused by the MT platform. the most practical way to do this is still use the http codes (hence your Error interface).
we could create an internal error type that can denote the difference between these two scenarios (and perhaps a few more) but invariably it would tie into http status codes anyway, so I figured, might as well put it with the rest of that stuff.
what's the alternative? create another internal error type outside of api/response, but it would still have to be able to set code 500, 400, etc (so practically it would still import the http package because it allows us to refer to http.StatusBadRequest and so forth). Would that be your preferred approach? (it would be mine but i abandoned that approach and i forgot why)
There was a problem hiding this comment.
create another internal error type outside of api/response
That would be my approach. It seems excessive to have to re-define an error struct that impliments the response.Error interface in every package. So maybe we just create a top level
metrictank/errors package and move instance.Error and instance.ErrorResp there (or just copy them for now and open another issue to update all of the api package to use it)
There was a problem hiding this comment.
assuming you meant response.Error instead of instance.Error, that's really an interface to describe an error type that has useful stuff in a http context. it makes sense for that to be in our api package which deals with all http stuff. not in the new errors package which tries to be generic and not tied to http.
that said, the new error types support the interface of course, but that's about as far as the errors package should go. I think this is in line with how/where interfaces are typically defined in go software. see 4212925 for the new version
it's legal for even the root node to not be in the tree, e.g. if everything was deleted. so we need to simply return no results in that case
helps #770
helps #811