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

Add cnt module - durable counters using sparse files #36

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add cnt module - durable counters using sparse files #36

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 4, 2015

a small offering, to dip my toe in the water

@dgibson
Copy link
Collaborator

dgibson commented Dec 4, 2015

On Thu, Dec 03, 2015 at 09:13:10PM -0800, dancancode wrote:

a small offering, to dip my toe in the water
You can view, comment on, or merge this pull request online at:

#36

-- Commit Summary --

  • Add cnt module - durable counters using sparse files

-- File Changes --

M .gitignore (1)
M Makefile-ccan (1)
A ccan/cnt/LICENSE (9)
A ccan/cnt/_info (77)
A ccan/cnt/cnt.c (164)
A ccan/cnt/cnt.h (87)
A ccan/cnt/test/run.c (103)

Ok.. it's not immediately clear to me why it's useful to store a
counter as a file size. Why not just store the actual value into the
file?

David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT the other
| way around!
http://www.ozlabs.org/~dgibson

@ghost
Copy link
Author

ghost commented Dec 4, 2015

I find them easier to observe from the command line. For example, I can read and sort a set of counters with 'ls -lS'.

@dgibson
Copy link
Collaborator

dgibson commented Dec 6, 2015

On Fri, Dec 04, 2015 at 06:49:11AM -0800, dancancode wrote:

I find them easier to observe from the command line. For example, I can read and sort a set of counters with 'ls -lS'.

Hm, ok.

Seems strange to me, but merely being esoteric is no reason not to
include it in ccan.

However, I think something using a method this unusual probably needs
a more specific name than "cnt".

David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT the other
| way around!
http://www.ozlabs.org/~dgibson

@ghost
Copy link
Author

ghost commented Dec 6, 2015

I find naming really hard. I'm open to suggestions.

@rustyrussell
Copy link
Owner

dancancode [email protected] writes:

I find naming really hard. I'm open to suggestions.

filecnt?

Cheers,
Rusty.

@dgibson
Copy link
Collaborator

dgibson commented Dec 10, 2015

On Sun, Dec 06, 2015 at 03:00:23AM -0800, Rusty Russell wrote:

dancancode [email protected] writes:

I find naming really hard. I'm open to suggestions.

filecnt?

Even that seems too generic to me, since encoding the counter in the
file size seems pretty weird to me even amounts things that put a
counter in a file somehow.

I'd go with "filesize_counter", even though it's kinda long.

I'd be ok with using that full string for the actual module name, but
something shorter for the namespacing prefix within ("fsc_"?
"szcnt_"?).

David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT the other
| way around!
http://www.ozlabs.org/~dgibson

* Renamed to filesize_counter with szcnt_ prefixes per David Gibson's suggestion
* Added some paragraphs to _info to demystify the code's raison d'etre
* Added tests to reach 100% coverage

Signed-off-by: Dan Good <[email protected]>
@ghost
Copy link
Author

ghost commented Dec 16, 2015

I gave it more spit and polish. Do you think it passes muster now?

@dgibson
Copy link
Collaborator

dgibson commented Feb 15, 2016

On Wed, Dec 16, 2015 at 12:09:11AM -0800, dancancode wrote:

I gave it more spit and polish. Do you think it passes muster now?

Sorry I never replied to this.

I think the code is fine now, but there are some issues in the actual
pull request. Most of it you can probably work out from your
experience since with other modules, but the basics things are:
- we need S-o-b lines
- each commit in the pull request should be reviewable; so you
want to fold together some of the fixup patches and clean up
- actually, rather than using a github pull request it's probably
simpler to made a clean set of patches to post to the list

Plus.. I've realised I made a terrible suggestion to you. Having
looked at it, my idea of using different long/short names for the
module and the function names within it was not a good idea at all.

Please use "szcnt" throughout.

David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT the other
| way around!
http://www.ozlabs.org/~dgibson

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.

2 participants