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-2645] Add ttl option for default write policies in client config #536

Merged
merged 25 commits into from
Nov 8, 2023

Conversation

juliannguyen4
Copy link
Collaborator

@juliannguyen4 juliannguyen4 commented Nov 6, 2023

Build wheels all passes: https://github.com/aerospike/aerospike-client-python/actions/runs/6792350139

Valgrind

This branch: https://github.com/aerospike/aerospike-client-python/actions/runs/6792351521/job/18465486663
Stage branch: https://github.com/aerospike/aerospike-client-python/actions/runs/6793233371/job/18467761697

No extra memory errors or leaks caused by these changes

C client API changes: aerospike/aerospike-client-c@600d2d2

Documentation:

Extra changes:

  • Breaking change: batch_operate() now takes in an optional ttl parameter instead of taking ttl through a batch policy. Documentation changes
  • apply policy now takes in a ttl option. Documentation
  • Add ttl option to aerospike.Scan class. Documentation
  • Documentation change: batch apply policies section now references TTL constants section instead of copying information from the latter. Documentation

@juliannguyen4 juliannguyen4 changed the base branch from master to stage November 6, 2023 21:17
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (3283cab) 81.06% compared to head (e45a0ff) 81.05%.
Report is 1 commits behind head on stage.

Additional details and impacted files
@@            Coverage Diff             @@
##            stage     #536      +/-   ##
==========================================
- Coverage   81.06%   81.05%   -0.01%     
==========================================
  Files          98       98              
  Lines       14828    14833       +5     
==========================================
+ Hits        12020    12023       +3     
- Misses       2808     2810       +2     
Files Coverage Δ
src/main/conversions.c 72.54% <100.00%> (+0.03%) ⬆️
src/main/policy.c 84.23% <ø> (ø)
src/main/policy_config.c 90.52% <100.00%> (+0.28%) ⬆️
src/main/scan/type.c 100.00% <ø> (ø)
src/main/client/batch_write.c 83.08% <50.00%> (-0.07%) ⬇️
src/main/client/operate.c 91.57% <75.00%> (-0.03%) ⬇️
src/main/client/batch_operate.c 82.38% <77.77%> (-1.60%) ⬇️

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

yuvalspinzi
yuvalspinzi previously approved these changes Nov 8, 2023
@juliannguyen4 juliannguyen4 marked this pull request as ready for review November 8, 2023 16:01
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.

Nice job with all the tests. Just a few questions. Mainly for my understanding, like why does batch operate need a new ttl argument if ttl is already a member of the batch write policy?

@@ -442,6 +443,7 @@ class Query:
def where(self, predicate: tuple, ctx: list = ...) -> None: ...

class Scan:
ttl: int
Copy link
Contributor

Choose a reason for hiding this comment

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

Do queries need this too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doc/client.rst Outdated Show resolved Hide resolved
doc/scan.rst Outdated Show resolved Hide resolved
src/main/client/batch_operate.c Show resolved Hide resolved
@juliannguyen4
Copy link
Collaborator Author

juliannguyen4 commented Nov 8, 2023

I fixed an extra doc error. Setting Scan.ttl to aerospike.TTL_CLIENT_DEFAULT should use the client's default scan policy, not it's default write policy. I confirmed this is correct with Brian

@juliannguyen4 juliannguyen4 force-pushed the CLIENT-2645-client-conf-ttl branch from dee00ae to d6dd722 Compare November 8, 2023 20:01
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.

Assuming tests pass this LGTM

@juliannguyen4 juliannguyen4 merged commit 4f37473 into stage Nov 8, 2023
16 of 17 checks passed
@juliannguyen4 juliannguyen4 deleted the CLIENT-2645-client-conf-ttl branch November 8, 2023 21:55
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.

4 participants