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

messageRateMax/messageRateMin documentation/implementation inconsistency #1097

Closed
reidsunderland opened this issue Jun 6, 2024 · 6 comments · Fixed by #1106
Closed

messageRateMax/messageRateMin documentation/implementation inconsistency #1097

reidsunderland opened this issue Jun 6, 2024 · 6 comments · Fixed by #1106
Labels
bug Something isn't working Documentation Primary deliverable of this item is documentation likely-fixed likely fix is in the repository, success not confirmed yet.

Comments

@reidsunderland
Copy link
Member

The documentation says that messageRaxMax/Min are float options https://metpx.github.io/sarracenia/Reference/sr3_options.7.html#messageratemax-float-default-0

But they're count options in the code:

count_options = [
'batch', 'count', 'exchangeSplit', 'instances', 'logRotateCount', 'no',
'post_exchangeSplit', 'prefetch', 'messageCountMax', 'messageRateMax',
'messageRateMin', 'runStateThreshold_reject', 'runStateThreshold_retry', 'runStateThreshold_slow'
]

float_options = [ ]

I agree with the documentation, messageRateMax should be a float because it allows more flexibility in the values we choose.

@reidsunderland reidsunderland added bug Something isn't working Documentation Primary deliverable of this item is documentation labels Jun 6, 2024
@petersilva
Copy link
Contributor

well that's confusing... I think it used to be a float and got converted... I think because count_options understand magnitude suffixes: k,m,g ... so 5kb -> 5120 bytes / second. ... 5k --> 5000 bytes/second ... I could swear I have used floating units in the past though...

OK, the routine to parse counts is called... wait for it... parse_count, and it used humanfriendly.parse_size...


>>> import humanfriendly as hf
>> hf.parse_size('4k', binary=True)
4096
>>> hf.parse_size('4.2k', binary=True)
4300
>>> hf.parse_size('0.2k', binary=True)
204
>>> hf.parse_size('0.2k', binary=False)
200
>>> 

we use the 'b' suffix to switch the binary on and off.

so I think even though it is a count, it does accept fractions.

@petersilva
Copy link
Contributor

petersilva commented Jun 7, 2024

def parse_count(cstr):
    if cstr[0] == '-':
        offset=1
    else:
        offset=0
    count=humanfriendly.parse_size(cstr[offset:], binary=cstr[-1].lower() in ['i','b'] )
    return -count if offset else count

I accept 'i' because there are some standards (I forget which) that use kilobytes for decimal and kibibytes for base 2... mibibytes vs. megabytes... etc... I can't get used to it, but apparently it's a thing... I figured i made sense.

@petersilva
Copy link
Contributor

so... I think that's why there are no float_options anymore. They can all accept the k,m,g suffixes so more convenient... maybe need better description in the docs though.

@reidsunderland
Copy link
Member Author

I think that makes sense in general. But messageRateMin/Max is messages/second and parse_size only returns integers.

There are cases where I'd like to rate limit to less than 1 message per second. I think those cases might be more frequent than cases needing some number of kilomessages per second? 😄

@petersilva
Copy link
Contributor

so... If it returns < 1, I could replace with a float value... but then 1.5 would be wrong...
so I could make a threshold of 1000... anything that's less than a 1000 would return a float.
but then batch ==50.0 ... strange. so then it looks like we should make count and float
separate again, and use the humanfriendly stuff as a helper for float... with 1000 as a threshold,
and use it without the helper for counts.

@petersilva
Copy link
Contributor

petersilva commented Jun 7, 2024

the problem with this 1000 threshold is if someone says 0.5k ... it will crash... 0.5k turns into 512 whith is less than 1000... then try to convert the string with a k in it, and boom ... ok need to adjust for that... but consider 0.0002mi to be abuse, and won't bother with it. ugh...

def parse_float(cstr):
    if type(cstr) is not str:
        return cstr

    fa = parse_count(cstr)
    if abs(fa) < 1000:
        if cstr[-1] in [ 'k', 'b', 'i' ]:
            if cstr[-1] in [ 'b', 'i' ]:
                fa=float(cstr[0:-2])*1024
            else:
                if cstr[-2] != 'k':
                    logger.error( f"malformed float: {cstr}, overriding to kilo" )
                fa=float(cstr[0:-1])*1000
        else:
            fa=float(cstr)
    return fa

@petersilva petersilva added the likely-fixed likely fix is in the repository, success not confirmed yet. label Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Documentation Primary deliverable of this item is documentation likely-fixed likely fix is in the repository, success not confirmed yet.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants