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

libkvs: compact only if ops len > 1 #3807

Merged
merged 1 commit into from
Aug 1, 2021

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Jul 31, 2021

I noticed this will doing some looking at issue #1804 awhile back. Forgot that I had this sitting on a branch. It's just a simple / dumb fix.

Problem: The txn_compact() function would attempt to compact
transactions if there are > 0 operations.  But it is impossible to
compact just 1 operation, you need atleast 2 operations to conceivably
compact.

Solution: Check for > 1 instead of > 0 operations.

@@ -210,7 +210,7 @@ int txn_compact (flux_kvs_txn_t *txn)
}

len = json_array_size (txn->ops);
if (!len)
if (!(len > 1))
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

Slightly odd comparison there - what about len < 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

haha,yeah I guess that would be simpler :-)

Problem: The txn_compact() function would attempt to compact
transactions if there are > 0 operations.  But it is impossible to
compact just 1 operation, you need atleast 2 operations to conceivably
compact.

Solution: Check for > 1 instead of > 0 operations.
@chu11
Copy link
Member Author

chu11 commented Aug 1, 2021

re-pushed per @garlick comment above. I'll set MWP after this passes.

@codecov
Copy link

codecov bot commented Aug 1, 2021

Codecov Report

Merging #3807 (0f8ddea) into master (a3f5f71) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3807      +/-   ##
==========================================
- Coverage   83.35%   83.33%   -0.02%     
==========================================
  Files         348      348              
  Lines       51014    51014              
==========================================
- Hits        42521    42513       -8     
- Misses       8493     8501       +8     
Impacted Files Coverage Δ
src/common/libkvs/kvs_txn_compact.c 71.75% <100.00%> (ø)
src/modules/job-archive/job-archive.c 59.27% <0.00%> (-0.81%) ⬇️
src/modules/job-info/guest_watch.c 76.00% <0.00%> (-0.62%) ⬇️
src/broker/overlay.c 88.97% <0.00%> (-0.56%) ⬇️
src/broker/content-cache.c 82.49% <0.00%> (-0.27%) ⬇️

@mergify mergify bot merged commit 53f37bb into flux-framework:master Aug 1, 2021
@chu11 chu11 deleted the issue1804_zhash branch August 2, 2021 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants