Skip to content

Conversation

@DomPeliniAerospike
Copy link
Collaborator

@DomPeliniAerospike DomPeliniAerospike commented Nov 14, 2025

There is missing code coverage for returning None from the Cluster class's app_id attribute, but that is fine because it is hard to reproduce that behavior and it's documented that as_cluster.app_id can be NULL in the C client

Extra changes:

  • Allow client config option app_id to be set to None.
  • Docs: fix app_id and cluster_names' types to be Optional[str] since they can be set to None.
  • Docs: set default value for client config's app_id and cluster_name option to None.
  • Docs: Update outdated documentation for the behavior of client config's cluster_name.
  • Make a standalone script for user agent test to make easier to run locally
  • In user agent standalone test, also check the server received the app_id from the Python client's client config

Docs

https://aerospike-python-client--864.org.readthedocs.build/en/864/aerospike.html#:~:text=meaning%20‘never%20compress’-,cluster_name,-(Optional[str
https://aerospike-python-client--864.org.readthedocs.build/en/864/aerospike_helpers.metrics.html#aerospike_helpers.metrics.Cluster.app_id

TODO

  • cluster_name description needs to be tested
  • app_id documentation needs improvement.
  • test case where app_id is not-set
  • script needs to clean up properly when cancelled

Manual testing

  • Code coverage looks good
  • Build artifacts passes
  • Valgrind shows no mem errors or leaks from these changes
  • Massif mem usage looks ok

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2025

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.37%. Comparing base (a60d3d9) to head (92bda5a).
⚠️ Report is 10 commits behind head on dev.

Files with missing lines Patch % Lines
src/main/conversions.c 70.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #864      +/-   ##
==========================================
- Coverage   83.38%   83.37%   -0.01%     
==========================================
  Files          99       99              
  Lines       14394    14406      +12     
==========================================
+ Hits        12002    12011       +9     
- Misses       2392     2395       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DomPeliniAerospike DomPeliniAerospike changed the title CLIENT-3805 [CLIENT-3805] Added app_id to cluster object for extended metrics Nov 20, 2025
@juliannguyen4 juliannguyen4 changed the title [CLIENT-3805] Added app_id to cluster object for extended metrics [CLIENT-3805] Add missing app_id to cluster object for extended metrics Dec 5, 2025
@juliannguyen4 juliannguyen4 changed the title [CLIENT-3805] Add missing app_id to cluster object for extended metrics [CLIENT-3805] Add missing app_id attribute to Cluster class for extended metrics Dec 5, 2025
@juliannguyen4 juliannguyen4 marked this pull request as ready for review January 9, 2026 00:27
Copy link
Contributor

@justinlee-aerospike justinlee-aerospike left a comment

Choose a reason for hiding this comment

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

Changes are okay

@juliannguyen4 juliannguyen4 dismissed their stale review January 9, 2026 00:49

Justin already reviewed and approved

@juliannguyen4 juliannguyen4 merged commit e4731b7 into dev Jan 9, 2026
128 of 130 checks passed
@juliannguyen4 juliannguyen4 deleted the CLIENT-3805 branch January 9, 2026 00:50
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.

5 participants