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

Concrete ls index name #1013

Merged
merged 3 commits into from
Jun 22, 2015

Conversation

ayashjorden
Copy link
Contributor

Tested '/' functionality as concrete ES index name signaling (without Bosun adding date suffix), works great.

Review on Reviewable

@@ -122,6 +122,12 @@ func (e *LogstashElasticHosts) GenIndices(r *LogstashRequest) (string, error) {
if err != nil {
return "", err
}
// Short-circut when using concrete ES index name
if len(r.IndexRoot) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the line below can be reduced to if strings.HasSuffix(r.IndexRoot, "/").

@ayashjorden
Copy link
Contributor Author

@mjibson, (as I'm new to GO) Should I reduce it?

@maddyblue
Copy link
Contributor

Yes, you should. But I'm a bit confused about the intended behavior. That will test if just the last character is a "/", not if the entire string has value "/". Which do you want?

@ayashjorden
Copy link
Contributor Author

As you can see in #974 , the intended behavior is to enable the user to signal Bosun 'just use this index name and don't add date suffix'.

The context behind this is that ES indices 'costs' quite a lot in terms of memory and cpu. there are use-cases such as the one that initiated this PR that the size of data is <5MB per day so there is no reason to create new indices every day. also there's the fact that olivere/elastic client (that Bosun is using to interact with ES) query's indices by index name, and doesn't include index aliases (which you can think of sym-links from 'myindex-2015.05.31' to 'myindex-global').

Will the 'strings.HasSuffix(r.IndexRoot, "/")' will handle nil vals gracefully?

Hope that the above is clear enoungh,
Yarden

@maddyblue
Copy link
Contributor

Strings can't be nil, but they can be empty, and HasSuffix handles that correctly.

@maddyblue
Copy link
Contributor

@kylebrandt, could you review this?

@kylebrandt
Copy link
Member

+1

maddyblue added a commit that referenced this pull request Jun 22, 2015
@maddyblue maddyblue merged commit 0e46b15 into bosun-monitor:master Jun 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants