ReadOnly option #541
ReadOnly option #541
Conversation
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
krasi-georgiev
force-pushed
the
readonly-db
branch
from
079daf4
to
86bb466
Compare
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
|
@krasi-georgiev - is there anything I can do to help with this? |
|
@glightfoot no thanks , I will finish this probably tomorrow. |
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>
|
ready for review |
|
ping @gouthamve , @codesome |
There was a problem hiding this comment.
Yo, initial review. I really like the direction of having read only TSDB - it is must have. But I think this might be totally separate struct we are looking for that exposes only READ methods like Series etc.. How hard would be to separate those vs injecting all those conditionals in the critical path. E.g I don't think we need to do reload even. Having totally separate code with some kind of composability for components that needs to be reused?
| opts := *tsdb.DefaultOptions | ||
| opts.NoLockfile = true | ||
| opts.ReadOnly = true | ||
| db, err = tsdb.Open(dir, l, r, &opts) |
There was a problem hiding this comment.
return tsdb.Open(dir, l, r, &opts) is not enough?
| @@ -62,9 +73,6 @@ func main() { | |||
| dumpMaxTime = dumpCmd.Flag("max-time", "maximum timestamp to dump").Default(strconv.FormatInt(math.MaxInt64, 10)).Int64() | |||
| ) | |||
|
|
|||
| safeDBOptions := *tsdb.DefaultOptions | |||
There was a problem hiding this comment.
I think I would prefer readOnlyOptions struct and pass it everywhere.
| "github.com/prometheus/tsdb" | ||
| "github.com/prometheus/tsdb/chunks" | ||
| "github.com/prometheus/tsdb/labels" | ||
| "gopkg.in/alecthomas/kingpin.v2" | ||
| ) | ||
|
|
||
| func openReadOnlyTSDB(dir string, l log.Logger, r prometheus.Registerer) (db *tsdb.DB, err error) { |
There was a problem hiding this comment.
Not sure why those logger and registerer opts if you never fill them (: I think it proves that just readOnlyOpts struct would be enough instead of this function
| @@ -79,6 +80,10 @@ type Options struct { | |||
| // Overlapping blocks are allowed if AllowOverlappingBlocks is true. | |||
| // This in-turn enables vertical compaction and vertical query merge. | |||
| AllowOverlappingBlocks bool | |||
|
|
|||
| // ReadOnly disables all mutating actions. | |||
There was a problem hiding this comment.
"Disables", means it will return error when acessing Appender methods etc?
|
|
||
| // ReadOnly disables all mutating actions. | ||
| // Useful for opening the same db more than once. | ||
| ReadOnly bool |
There was a problem hiding this comment.
Have we considered totally different constructor instead returning limited TSDB interface ?
| @@ -588,8 +598,10 @@ func (db *DB) reload() (err error) { | |||
| } | |||
| } | |||
|
|
|||
| if err := db.deleteBlocks(deletable); err != nil { | |||
| return err | |||
| if !db.opts.ReadOnly { | |||
There was a problem hiding this comment.
All those ifs in this file. Do we really need to invoke reload and exactly the same Open functions in readOnly TSDB mode?
What readOnly mode should give? I think only tiny shim for Querying, right? Can we just add separate layer that does just that? It would massively reduce extra complexity in critical code here. What do you think? (:
|
superseded by #588 |
just continuing the work from #517
fixes #516
Signed-off-by: Krasi Georgiev kgeorgie@redhat.com