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.

Sign up for GitHub

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

Jump to bottom

improve scorch config parsing of forced zap version #1401

Merged
merged 2 commits into from
May 26, 2020
Merged

improve scorch config parsing of forced zap version #1401

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a7746a
improve scorch config parsing of forced zap version
mschoch May 22, 2020
3b0a5ea
rename ok2 to ok
mschoch May 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 10 additions & 9 deletions index/scorch/scorch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,17 +139,18 @@ func NewScorch(storeName string,
// configForceSegmentTypeVersion checks if the caller has requested a
// specific segment type/version
func configForceSegmentTypeVersion(config map[string]interface{}) (string, uint32, error) {
forcedSegmentVersion, ok := config["forceSegmentVersion"].(int)
if ok {
forcedSegmentType, ok2 := config["forceSegmentType"].(string)
if !ok2 {
return "", 0, fmt.Errorf(
"forceSegmentVersion set to %d, must also specify forceSegmentType", forcedSegmentVersion)
}
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?

}

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

if !ok2 {
return "", 0, fmt.Errorf(
"forceSegmentVersion set to %d, must also specify forceSegmentType", forcedSegmentVersion)
}
return "", 0, nil

return forcedSegmentType, uint32(forcedSegmentVersion), nil
}

func (s *Scorch) paused() uint64 {
Expand Down