[dbnode] Remove coldwrites error message until feature is ready #1651
Conversation
| @@ -79,7 +79,7 @@ type databaseBuffer interface { | |||
| id ident.ID, | |||
| tags ident.Tags, | |||
| persistFn persist.DataFn, | |||
| nsCtx namespace.Context, | |||
| nsCtx namespace.Context, | |||
There was a problem hiding this comment.
This was poorly formatted before.
src/dbnode/storage/series/buffer.go
Outdated
| /* | ||
| Disable until cold writes is ready as this is confusing to users. | ||
| if !b.coldWritesEnabled { | ||
| return false, m3dberrors.ErrColdWritesNotEnabled |
There was a problem hiding this comment.
We should still return an error here, otherwise a write that's too old will look like it got written successfully. How about just changing the error message to something like "datapoint is too far in the past or future"?
There was a problem hiding this comment.
But for folks who have it disabled (everyone), won't the writeType equal ColdWrite for all writes with timestamp outside of the buffer? By removing this, if we get an old time stamp, won't we return one of the errors in the following lines? (Granted there is a small window for race condition between the two checks, but I assume our buffers can accept some variability there).
I can also just remove the writeType check completely, but the type is passed beyond this func.
There was a problem hiding this comment.
The checks in the following lines are the limits of ColdWrites and so checks using retentionPeriod/futureRetentionPeriod, instead of bufferPast/bufferFuture.
There was a problem hiding this comment.
Ah missed that. Ok, I'll just change the original error msg then.
src/dbnode/storage/errors/types.go
Outdated
| @@ -35,9 +35,10 @@ var ( | |||
| ErrTooPast = xerrors.NewInvalidParamsError(errors.New("datapoint is too far in the past")) | |||
|
|
|||
| // ErrColdWritesNotEnabled is returned when cold writes are disabled | |||
| // and a write is too far in the past or future. | |||
| // and a write is too far in the past or future. Note, the error intentionally | |||
| // excludes anything regarding the cold writes feature until it's release. | |||
What this PR does / why we need it:
When any datapoint outside of the buffer is currently written, the user gets back a msg saying that the coldWrites feature (which is not ready) is not enabled. This causes a lot of confusion.