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

node: stress_object: strip off commas from value strings #543

Merged
merged 2 commits into from
Dec 24, 2023

Conversation

bhalevy
Copy link
Member

@bhalevy bhalevy commented Dec 17, 2023

Some resharding_test dtest, e.g. test_resharding_counter fails locally for me:

    def _run_stress(self, op_cnt, stress_cmd):
        res = self.node.stress_object(stress_cmd)
        if not isinstance(res, dict):
            raise Exception('Error running cassandra-stress: {}'.format(res))
        assert res['total errors'] == 0
>       assert res['total partitions'] >= op_cnt
E       assert 10.0 >= 10000

When printing the cassandra-stress stdout I saw that it uses a comma as a 1000s separator:

Total partitions          :     10,000 [COUNTER_READ: 10,000]

This is probably related to the LOCALE on my machine.

This change explicitly sets the locale to en_US
tha defines comma as the thousands separator
and dot as the decimal point.

The origin local is reset when the function is done.

@bhalevy bhalevy requested a review from fruch December 17, 2023 11:07
Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

It's not clear why the locale is even involved in this code

I would remove it completely

@bhalevy
Copy link
Member Author

bhalevy commented Dec 17, 2023

It's not clear why the locale is even involved in this code

I would remove it completely

I think that the intention was to try to match the locale settings cassandara-stress uses to print its output numbers.
Some locales, like de_DE use comma as a decimal point and dot as the thousands separator.
But then, setting the locale should be done before running stress, and the previous locale can be restored after results are parsed.
This was way maybe it'd be best to set the locale to en_US. I'll try that

@bhalevy
Copy link
Member Author

bhalevy commented Dec 17, 2023

Yup, setting the locale explicitly to en_US works well as it defines
the thousands separator to comma.

@bhalevy bhalevy changed the title node: stress_object: ignore commas in numbers node: stress_object: set locale to en_US Dec 17, 2023
ccmlib/node.py Outdated
start = True
prev_locale, encoding = locale.getlocale(locale.LC_NUMERIC)
# Set locale to en_US to simplify printing and parsing of numbers
locale.setlocale(locale.LC_NUMERIC, 'en_US')
Copy link
Contributor

Choose a reason for hiding this comment

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

if we hard code to a specific one,

we can just replace(',',''), and remove all locale related code

setting the locale like that, might break other code we aren't even aware of, like third party code.

@fruch
Copy link
Contributor

fruch commented Dec 20, 2023

@bhalevy this change also introduce 3 more items into the object, which the unittests doesn't know about (and fails)

@bhalevy
Copy link
Member Author

bhalevy commented Dec 20, 2023

@bhalevy this change also introduce 3 more items into the object, which the unittests doesn't know about (and fails)

I dropped this part in the latest version (265d8fe).
I thought this could help debug future issues but its not crucial right now.

@bhalevy
Copy link
Member Author

bhalevy commented Dec 20, 2023

Hmm

=================================== FAILURES ===================================
________________ TestScyllaRelocatableCluster.test_node_stress _________________

self = <tests.test_scylla_relocatable_cluster.TestScyllaRelocatableCluster object at 0x7fc0b4a16820>
relocatable_cluster = <ccmlib.scylla_cluster.ScyllaCluster object at 0x7fc0b3d2ecd0>

    def test_node_stress(self, relocatable_cluster):
        node1, *_ = relocatable_cluster.nodelist()
        node1: Node
        ret = node1.stress(['write', 'n=10'])
        assert '10 [WRITE: 10]' in ret.stdout
        assert 'END' in ret.stdout
    
>       ret = node1.stress_object(['write', 'n=10'])

tests/test_scylla_relocatable_cluster.py:36: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
ccmlib/node.py:1408: in stress_object
    locale.setlocale(locale.LC_NUMERIC, 'en_US')
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

category = 1, locale = 'en_US'

    def setlocale(category, locale=None):
    
        """ Set the locale for the given category.  The locale can be
            a string, an iterable of two strings (language code and encoding),
            or None.
    
            Iterables are converted to strings using the locale aliasing
            engine.  Locale strings are passed directly to the C lib.
    
            category may be given as one of the LC_* values.
    
        """
        if locale and not isinstance(locale, _builtin_str):
            # convert to string
            locale = normalize(_build_localename(locale))
>       return _setlocale(category, locale)
E       locale.Error: unsupported locale setting

/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/locale.py:608: Error

@fruch
Copy link
Contributor

fruch commented Dec 20, 2023

Hmm

=================================== FAILURES ===================================
________________ TestScyllaRelocatableCluster.test_node_stress _________________

self = <tests.test_scylla_relocatable_cluster.TestScyllaRelocatableCluster object at 0x7fc0b4a16820>
relocatable_cluster = <ccmlib.scylla_cluster.ScyllaCluster object at 0x7fc0b3d2ecd0>

    def test_node_stress(self, relocatable_cluster):
        node1, *_ = relocatable_cluster.nodelist()
        node1: Node
        ret = node1.stress(['write', 'n=10'])
        assert '10 [WRITE: 10]' in ret.stdout
        assert 'END' in ret.stdout
    
>       ret = node1.stress_object(['write', 'n=10'])

tests/test_scylla_relocatable_cluster.py:36: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
ccmlib/node.py:1408: in stress_object
    locale.setlocale(locale.LC_NUMERIC, 'en_US')
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

category = 1, locale = 'en_US'

    def setlocale(category, locale=None):
    
        """ Set the locale for the given category.  The locale can be
            a string, an iterable of two strings (language code and encoding),
            or None.
    
            Iterables are converted to strings using the locale aliasing
            engine.  Locale strings are passed directly to the C lib.
    
            category may be given as one of the LC_* values.
    
        """
        if locale and not isinstance(locale, _builtin_str):
            # convert to string
            locale = normalize(_build_localename(locale))
>       return _setlocale(category, locale)
E       locale.Error: unsupported locale setting

/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/locale.py:608: Error

As I said, we don't know which locale are available where we're gonna run, we shouldn't be setting it inside a library code.

@bhalevy
Copy link
Member Author

bhalevy commented Dec 20, 2023

Ok, I'll go back to square one

Some resharding_test dtest, e.g. `test_resharding_counter` fails locally for me:
```
    def _run_stress(self, op_cnt, stress_cmd):
        res = self.node.stress_object(stress_cmd)
        if not isinstance(res, dict):
            raise Exception('Error running cassandra-stress: {}'.format(res))
        assert res['total errors'] == 0
>       assert res['total partitions'] >= op_cnt
E       assert 10.0 >= 10000
```

When printing the cassandra-stress stdout I saw that it
uses a comma as a 1000s separator:
```
Total partitions          :     10,000 [COUNTER_READ: 10,000]
```

This is probably related to the LOCALE on my machine.

This change uses the current locale for locale.atof
but it deletes any commas from the numbers.

Signed-off-by: Benny Halevy <[email protected]>
@bhalevy bhalevy changed the title node: stress_object: set locale to en_US node: stress_object: strip off commas from value strings Dec 21, 2023
@bhalevy
Copy link
Member Author

bhalevy commented Dec 21, 2023

@fruch please merge

ccmlib/node.py Outdated Show resolved Hide resolved
Co-authored-by: Israel Fruchter <[email protected]>
@bhalevy bhalevy requested a review from fruch December 22, 2023 06:46
Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

@fruch fruch merged commit ed69ccb into scylladb:next Dec 24, 2023
3 checks passed
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.

2 participants