-
Notifications
You must be signed in to change notification settings - Fork 152
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
Changes all places where substring filter is named "substr" to "sub" for consistency #419
Conversation
docs/config.md
Outdated
@@ -73,7 +73,7 @@ setting | mandatory | values | def | |||
key | Y | string | N/A | | |||
type | Y | sendAllMatch/sendFirstMatch/consistentHashing | N/A | send to all destinations vs first matching destination vs distribute via consistent hashing | |||
prefix | N | string | "" | | |||
sub | N | string | "" | | |||
substr | N | string | "" | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh?
this token is declared in imperatives.go
{Token: optSub, Pattern: "sub="},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm right, this seems to be a discrepancy with the config file parsing then....
because this route in the config file:
[[route]]
key = 'carbon-default'
type = 'sendFirstMatch'
sub = 'testvalue'
destinations = [
'127.0.0.1:2003 pickle=false'
]
gets printed like this when crng starts
## Routes:
type key prefix substr regex
============================================================================================
> sendFirstMatch carbon-default
prefix substr regex addr spoolDir spool pickle online
------------------------------------------------------------------------------------------
127.0.0.1:2003 /var/spool/carbon-relay-ng false false false
while this route in the config file:
[[route]]
key = 'carbon-default'
type = 'sendFirstMatch'
substr = 'testvalue'
destinations = [
'127.0.0.1:2003 pickle=false'
]
gets printed like this:
## Routes:
type key prefix substr regex
============================================================================================
> sendFirstMatch carbon-default testvalue
prefix substr regex addr spoolDir spool pickle online
------------------------------------------------------------------------------------------
127.0.0.1:2003 /var/spool/carbon-relay-ng false false false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should consistently use the string sub
everywhere, not substr
, because it is shorter and it's still quite obvious what it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the best way to change this from sub
to substr
? Should I make it backwards compatible, so it parses both correctly while giving sub
preference in case both are defined? Or should I make a breaking change where substr
doesn't get parsed anymore? I'd lean towards the former, otherwise we just annoy users unnecessarily, and implementing backwards compatibility would be very simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backwards compatibility is important.
BUT..
i think we should consistently use the string sub everywhere, not substr,
vs
What's the best way to change this from sub to substr?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, i wrote it the wrong way around, i meant What's the best way to change this from substr to sub?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.