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

refactor serializeLinks data chan #102

Closed
wants to merge 1 commit into from

Conversation

b5
Copy link

@b5 b5 commented Jun 27, 2017

👋

As promised, here's a very small first step on refactoring memgator. I've intentionally kept the changes very light, as we don't have a test suite in place.

This PR refactors the dataCh argument to serializeLinks, changing it's type from dataCh chan string to dataCh chan fmt.Stringer. All processes that read from dataCh now call .String() on the result to get the string they would previously output.

Which may seem like a small change, it opens up a number of improvements down the road, (which we can make with incremental PRs). Instead of the channel needing to accept an explicit string, it now accepts anything that can produce a string, we end up using this new feature immediately in the serializeLinks func, passing back raw buffers (bytes.Buffer has a built-in String() string method). And this sets the stage for returning one or more custom structs, each with a string method that cleans up the code inside the formatting switch of serializeLinks.

I'd recommend checking extensively on your end to make sure I haven't broken anything ☺️, but if you're into this PR there are a number of change we can make from here as a team. Feedback of all kinds welcomed.

@machawk1
Copy link
Member

machawk1 commented Jul 7, 2017

@ibnesayeed Any updates in evaluating this PR?

@ibnesayeed
Copy link
Member

The only reason why it is not merged yet is the opportunity to learn more from this. I haven't got a chance to look into the code in more details and test the changes. I am targeting it to be evaluated this weekend.

@ibnesayeed
Copy link
Member

Finally, I managed some cycles to look into this PR. As explained before, I did not merge it before because I wanted to understand what did it bring to the table and possible consequences it might have. I certainly learned some cool tricks from this PR.

One fundamental difference that this PR has made is that it now accumulates entire aggregated TimeMap as a string in a buffer then passes it to the channel which is then written out to the network IO or STDOUT. However, existing approach of pushing every serialized line to the channel gives the advantage of writing them out to the network IO or STDOUT immediately which will consume less memory (these TimeMaps sometimes are really big, so memory is a serious concern here). The streaming mode as per #95 and the CLI mode would suffer the most from this approach. Please correct me if I am mistaken in understanding this.

The response.go file (that is not being used yet) would serve as a cool reference when we start splitting the code into separate files or packages based on their responsibilities.

Apart from that, a memgator binary has also made into the commit, which is undesired.

Thanks @b5 for this quick, but very interesting PR. Irrespective of the final decision on its merger, there is no denial that I learned a few things from this.

@b5
Copy link
Author

b5 commented Feb 8, 2018

Thanks @ibnesayeed! closing 😄

@b5 b5 closed this Feb 8, 2018
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