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

improve scorch config parsing of forced zap version #1401

Merged
merged 2 commits into from May 26, 2020

Conversation

mschoch
Copy link
Member

@mschoch mschoch commented May 22, 2020

previous version did not correctly handle integer
values stored in a float64 due to JSON parsing

previous version did not correctly handle integer
values stored in a float64 due to JSON parsing

return forcedSegmentType, uint32(forcedSegmentVersion), nil
forcedSegmentType, ok2 := config["forceSegmentType"].(string)
Copy link
Member

Choose a reason for hiding this comment

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

Now that there's no "ok" anymore, "ok2" should be the new "ok" :-D

}
forcedSegmentVersion, err := parseToInteger(config["forceSegmentVersion"])
if err != nil {
return "", 0, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

in case of err, its returning nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because error in this case just means that it was not int or float64, in which case we do not proceed to process the settings (which I believe is consistent with what we did before).

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not very sure of this. Giving some hints about the incorrect format looks cleaner to me than not giving any clues to the user of what is going wrong.
Also, unable to comprehend the importance of maintaining consistency with what we did before for a small period of time between v11 - v12/13..

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not arguing for consistency, I'm simply saying I'm not changing the behavior (other than adding support for float64). If you'd like to improve the error handling, maybe you should create your own PR?

Copy link
Contributor

@sreekanth-cb sreekanth-cb left a comment

Choose a reason for hiding this comment

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

minor change

code review feedback
@mschoch
Copy link
Member Author

mschoch commented May 26, 2020

Just wasted half a day yesterday testing, because this PR hasn't been merged, bleve-blast wasn't using my forceSegmentVersion configuration.

@mschoch mschoch merged commit 0c90a2f into master May 26, 2020
@mschoch mschoch deleted the improve-force-zap-version-parsing branch May 26, 2020 15:36
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.

None yet

3 participants