Skip to content

(improvement) log a warning when importing of lz4 or snappy packages … #510

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mykaul
Copy link

@mykaul mykaul commented Aug 6, 2025

…fails.

If it is not available, the driver will silently not use compression. Not very very silently, as you will see in debug level only something like: "No available compression types supported on both ends. locally supported: odict_keys([]). remotely supported: ['lz4', 'snappy']"

Make it a warning. I think it wouldn't be too noisy and is clear enough for the developer.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@mykaul mykaul requested a review from Copilot August 6, 2025 14:32
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves user experience by adding warning logs when optional compression packages (lz4 and snappy) fail to import. Previously, missing compression packages would only show debug-level messages when compression negotiation failed, making it difficult for developers to understand why compression wasn't working.

  • Added warning log messages when lz4 package import fails
  • Added warning log messages when snappy package import fails

@Lorak-mmk
Copy link

One more thing we could (and should!) do is to specify those dependencies - right now they are totally absent from pyproject.toml, which makes them hard to discover.
Those should be listed in [project.optional-dependencies] section. We could either make it a single extra, like compression = ['lz4', 'snappy'], or extra-per-lib, like

compression-snappy = ['snappy']
compression-lz4 = ['lz4']

@mykaul
Copy link
Author

mykaul commented Aug 6, 2025

One more thing we could (and should!) do is to specify those dependencies - right now they are totally absent from pyproject.toml, which makes them hard to discover. Those should be listed in [project.optional-dependencies] section. We could either make it a single extra, like compression = ['lz4', 'snappy'], or extra-per-lib, like

compression-snappy = ['snappy']
compression-lz4 = ['lz4']

Yes, that's a different issue - it also means we never test compression.

@mykaul mykaul force-pushed the log_on_import_failure branch from 7079a15 to 7db823d Compare August 6, 2025 14:54
@mykaul
Copy link
Author

mykaul commented Aug 7, 2025

MacOS failure:

=================================== FAILURES ===================================
  __________________ StrategiesTest.test_nts_token_performance ___________________
  
  self = <tests.unit.test_metadata.StrategiesTest testMethod=test_nts_token_performance>
  
      def test_nts_token_performance(self):
          """
          Tests to ensure that when rf exceeds the number of nodes available, that we dont'
          needlessly iterate trying to construct tokens for nodes that don't exist.
      
          @since 3.7
          @jira_ticket PYTHON-379
          @expected_result timing with 1500 rf should be same/similar to 3rf if we have 3 nodes
      
          @test_category metadata
          """
      
          token_to_host_owner = {}
          ring = []
          dc1hostnum = 3
          current_token = 0
          vnodes_per_host = 500
          for i in range(dc1hostnum):
      
              host = Host('dc1.{0}'.format(i), SimpleConvictionPolicy)
              host.set_location_info('dc1', "rack1")
              for vnode_num in range(vnodes_per_host):
                  md5_token = MD5Token(current_token+vnode_num)
                  token_to_host_owner[md5_token] = host
                  ring.append(md5_token)
              current_token += 1000
      
          nts = NetworkTopologyStrategy({'dc1': 3})
          start_time = timeit.default_timer()
          nts.make_token_replica_map(token_to_host_owner, ring)
          elapsed_base = timeit.default_timer() - start_time
      
          nts = NetworkTopologyStrategy({'dc1': 1500})
          start_time = timeit.default_timer()
          nts.make_token_replica_map(token_to_host_owner, ring)
          elapsed_bad = timeit.default_timer() - start_time
          difference = elapsed_bad - elapsed_base
  >       assert difference < 1 and difference > -1
  E       assert (-1.130967961999886 < 1 and -1.130967961999886 > -1)

@mykaul
Copy link
Author

mykaul commented Aug 7, 2025

Looks like a flaky test to me. I have no idea what the test does - measure it's not more than 1 s difference? not sure who set it as such. Anyway... I can retry or whatnot.

mykaul added a commit to mykaul/python-driver that referenced this pull request Aug 7, 2025
While trying to look at some random (flaky?) test (scylladb#510 (comment) )
I saw some (minor) improvements that can be made to make_token_replica_map():
1. Remove some redundant len() calls to outside the loop(s)
2. Align some variable names, start with num_ ... for them.
3. Move token_offset and host assignment within the loop to closer to where it's used.

All those are probably very very minor improvements, perhaps in a large cluster it'll be noticable.

Signed-off-by: Yaniv Kaul <[email protected]>
@dkropachev
Copy link
Collaborator

dkropachev commented Aug 7, 2025

@mykaul , can you please also bump log level to error for case when user picked compression, but there is no compression library available:

log.debug("No available compression types supported on both ends."
" locally supported: %r. remotely supported: %r",
locally_supported_compressions.keys(),
remote_supported_compressions)

It is probably makes sense to reduce log level for import errors to debug, warning is too high IMHO.

mykaul added a commit to mykaul/python-driver that referenced this pull request Aug 8, 2025
While trying to look at some random (flaky?) test (scylladb#510 (comment) )
I saw some (minor) improvements that can be made to make_token_replica_map():
1. Remove some redundant len() calls to outside the loop(s)
2. Align some variable names, start with num_ ... for them.
3. Move token_offset and host assignment within the loop to closer to where it's used.
4. Only add DCs and hosts that are in the map

All those are probably very very minor improvements, perhaps in a large cluster it'll be noticable.

Signed-off-by: Yaniv Kaul <[email protected]>
…fails.

If it is not available, the driver will silently not use compression.
Not very very silently, as you will see in debug level only something like:
"No available compression types supported on both ends. locally supported: odict_keys([]). remotely supported: ['lz4', 'snappy']"

Make this log line a an error level log.

Add to the import failure a debug level log. I think it wouldn't be too noisy and is clear enough for the developer.

Signed-off-by: Yaniv Kaul <[email protected]>
@mykaul mykaul force-pushed the log_on_import_failure branch from 7db823d to 6a5fab5 Compare August 8, 2025 06:33
@mykaul
Copy link
Author

mykaul commented Aug 8, 2025

@mykaul , can you please also bump log level to error for case when user picked compression, but there is no compression library available:

log.debug("No available compression types supported on both ends."
" locally supported: %r. remotely supported: %r",
locally_supported_compressions.keys(),
remote_supported_compressions)

It is probably makes sense to reduce log level for import errors to debug, warning is too high IMHO.

Done.

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.

3 participants