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

docs: add some documentation of the segment format and creation #6

Merged
merged 11 commits into from
Sep 14, 2016
Merged

docs: add some documentation of the segment format and creation #6

merged 11 commits into from
Sep 14, 2016

Conversation

fgrosse
Copy link
Contributor

@fgrosse fgrosse commented Sep 12, 2016

I basically started with the example.go and wrote down what I understood from looking at the code. Before I go any further with this I wanted to submit a small pull request to get your feedback on this approach.

There is mostly just documentation going on and there should definitively be no logic change whatsoever but there are some instances were I renamed a variable to make it easier to see what is happening in the code.

Additionally I added some TODOs and since I'm very new to netlog its very possible that I have gotten something wrong so this will very likely have a follow up commit to fix things.

Let me know what you think
Friedrich :)

I basically wrote down what I understood from working through the code
to get a feeling of what it does. I added some TODOs and might have
gotten something wrong so this will very likely have a follow up commit
to fix things.
@fgrosse
Copy link
Contributor Author

fgrosse commented Sep 12, 2016

Please note that I applied the "Comment Sentences" hint from the official CodeReviewComments wherever possible.

@fgrosse
Copy link
Contributor Author

fgrosse commented Sep 12, 2016

I also payed some attention to the "Named Result Parameters" hint.

// Split generates a new segment recipient of future writes
func (bl *BigLog) Split() (err error) {
// Split creates a new segment in bl's dirPath using the maxIndexEntries of
// the currently active segment starting at the highest available offset+1.
Copy link
Contributor

Choose a reason for hiding this comment

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

using the maxIndexEntries of the currently active segment

that's kind of an implementation detail, the gist is that the maxIndexEntries remains the same, it could have been a fixed setting but since BigLog doesn’t have settings by itself I chose to derive it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about this?

// Split creates a new segment in bl's dirPath starting at the highest
// available offset+1. The new segment has the same size as the old one
// and becomes the new hot (active) segment.

Alternatively its probably also ok here to omit the segment size altogether. This kind of information could be documented in the high level package documentation describing the file layout of a Biglog.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@bictorman
Copy link
Contributor

Niiiice! Let me know if you need any ideas on what to tackle.
Welcome to the team Friedrich! 👍

@fgrosse
Copy link
Contributor Author

fgrosse commented Sep 13, 2016

😄 yeah I kind of got inspired by watching your golang uk talk yesterday. I'm gonna continue to review and understand the code before I can even think of implementing something.

Anyway it would probably be helpful to have some kind of platform to talk asynchronously about the code (apart from github issues). Any ideas? so far I have used gitter and google groups for this. If you feel this is overkill at this point I'll probably just write an email.

@bictorman
Copy link
Contributor

Happy to hear that people are able to parse the firehose of words that the talk turned out to be 😅

I would not mind a google group netlog-dev, that would not require an extra account and the emails are still public for future reference.

@fgrosse
Copy link
Contributor Author

fgrosse commented Sep 13, 2016

A google groups sounds good. As package owner I'll leave it up to you to create and probably also announce it in the README.md

@fgrosse fgrosse mentioned this pull request Sep 14, 2016
This will eventually be fixed in another pull request.
Maybe this will be done via netlog/issues#7
@bictorman bictorman merged commit b063449 into ninibe:master Sep 14, 2016
@bictorman bictorman mentioned this pull request Dec 27, 2017
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