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

[CLIENT-3106] Refactor aerospike module initialization code and check if error indicator is set after every C-API call #667

Merged
merged 45 commits into from
Sep 25, 2024

Conversation

juliannguyen4
Copy link
Collaborator

@juliannguyen4 juliannguyen4 commented Sep 9, 2024

Extra changes:

  • Consolidate all aerospike module constants into the same file and list
  • Remove aerospike_log_level enum because an enum already exists in C client's common submodule
  • Remove aerospike module state since the module itself already has the same members as Python attributes. No reason to store them twice as Python attributes and in the module state

TODO

  • Massif memory usage looks ok
  • No Valgrind memory errors
  • Build artifacts passes (with exception to noise)
  • Time performance looks good
# this branch
(.venv) jnguyen@Aerospikes-MacBook-Pro-4 dist % python3 -m timeit -n 50000 "import aerospike"
50000 loops, best of 5: 73.9 nsec per loop
# 15.1.0
(.venv) jnguyen@Aerospikes-MacBook-Pro-4 dist % python3 -m timeit -n 50000 "import aerospike"
50000 loops, best of 5: 73.7 nsec per loop

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 12 lines in your changes missing coverage. Please review.

Project coverage is 80.89%. Comparing base (d1e7bc2) to head (216969f).
Report is 14 commits behind head on dev.

Files with missing lines Patch % Lines
src/main/aerospike.c 68.42% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #667      +/-   ##
==========================================
- Coverage   80.96%   80.89%   -0.08%     
==========================================
  Files         100      100              
  Lines       15130    15053      -77     
==========================================
- Hits        12250    12177      -73     
+ Misses       2880     2876       -4     
Flag Coverage Δ
80.89% <69.23%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@juliannguyen4 juliannguyen4 force-pushed the CLIENT-3106-aerospike-module-refactor branch from 6ed50e3 to 9ce9f15 Compare September 11, 2024 15:45
@juliannguyen4 juliannguyen4 marked this pull request as ready for review September 16, 2024 19:33
@juliannguyen4 juliannguyen4 removed the request for review from dwelch-spike September 17, 2024 16:15
@juliannguyen4 juliannguyen4 changed the title [CLIENT-3106] Refactor aerospike module initialization code [CLIENT-3106] Refactor aerospike module initialization code and check if error indicator is set after every C-API call Sep 23, 2024
@juliannguyen4 juliannguyen4 removed the request for review from dwelch-spike September 24, 2024 21:28
Copy link
Contributor

@dwelch-spike dwelch-spike left a comment

Choose a reason for hiding this comment

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

Just a few double check questions. After you have had a look this LGTM

src/main/aerospike.c Show resolved Hide resolved
src/main/aerospike.c Show resolved Hide resolved
Copy link
Contributor

@dwelch-spike dwelch-spike left a comment

Choose a reason for hiding this comment

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

LGTM

@juliannguyen4 juliannguyen4 merged commit 8ff0327 into dev Sep 25, 2024
192 of 209 checks passed
@juliannguyen4 juliannguyen4 deleted the CLIENT-3106-aerospike-module-refactor branch September 25, 2024 22:36
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