Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong storage engine flag value #2087

Closed
fabxc opened this Issue Oct 17, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@fabxc
Copy link
Member

fabxc commented Oct 17, 2016

When we discussed a boolean enable/disable flag for local storage, we decided to have an engine string flag instead, which allows disabled as well as any custom engine people want.
However, the default value for the current local storage engine ended up being persistet, which is equivalent to a boolean flag again.

Anyway, changing it would be breaking, so we have to live with it.
Issue just helps tracking for >1.0.

@juliusv

@brian-brazil brian-brazil added this to the 2.x milestone Oct 26, 2016

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Jun 15, 2017

I suggest moving everything to -storage.tsdb.*. That's mostly done aside from -storage.local.path I think.

Do we really need a persistence-disabled mode for tsdb?
It came up before and I just suggested to point Prometheus to tmpdir and set retention == block size. We can even optimize that to drop in-memory blocks before their initial compaction if they are already behind the retention.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 15, 2017

I've never seen much use for persistence disabled, the use case always really seems to be for a very short retention period.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Jun 15, 2017

I think it's mainly used for Cortex collectors.

@gouthamve

This comment has been minimized.

Copy link
Member

gouthamve commented Jun 16, 2017

So, I saw that with none, we have a noopStorage which doesn't ingest anything and returns empty values.

Implementing the noopStorage is straight-forward but I think it should ideally be done after #2850 We could remove the flag now and add it when noopStorage is added.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Jun 27, 2017

This is gone for now. Pointing TSDB at tmpfs should work just fine. Truly preventing any file from being written has no strong enough performance benefit that'd be worth the effort right now.

@grobie

This comment has been minimized.

Copy link
Member

grobie commented Jun 27, 2017

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Jun 27, 2017

Would still be nice to be able to use Prometheus as a pure remote reader/writer. Otherwise one has to think how large a block on disk can get and how much memory it will consume... although all one wants to do is forward / read back samples. I mean, I'm not actively working on Cortex anymore, but think the idea is nice in general.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Jun 27, 2017

@juliusv yea, I see no problem adding it back once someone comes claims a need for it. But since we stirred up all the flags anyway, I don't see it as a hard 2.0 requirement.

One might even argue that this is more of a use case for the component disaggregation you started a discussion on a while ago. A forward-only mode should possibly rather be a sub-command that's explicit about the remaining functionality rather than just disabling the storage and being left with all query and rule APIs still enabled but non-functional.
Then you can just assemble your necessary components together in another main.go instead of accommodating different use cases into a single one, having some flags taking effect some not, etc.

That might mostly be a perception of purity though.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Jun 27, 2017

Though for the user it would be a straightforward thing to do. Instead of:

prometheus --storage.tsdb.disabled --config.file=abc.yml

you just say

prometheus forward --config.file=abc.yml
@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Jun 27, 2017

@fabxc That sounds good in general, though I wonder what that approach would look like if you still do want the querying (like when you use it for remote write and read). For that kind of thing, it'd be easier to specifically just turn the local storage off. But I also think it's ok to start 2.x without this and add it later. Better than adding something half-thought-out now that we can't break later.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Jun 27, 2017

Right, good point. Let's look into this again later then. Will remove it from the milestone for now.

@fabxc fabxc removed this from the v2.x milestone Jun 27, 2017

@fabxc fabxc closed this in 67dc73f Oct 7, 2017

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 23, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 23, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.