search

oduwsdl / MemGator

A Memento Aggregator CLI and Server in Go
https://memgator.cs.odu.edu/api.html
MIT License
56 stars 11 forks source link

refactor serializeLinks data chan #102

Closed b5 closed 6 years ago

b5 commented 7 years ago

👋

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 commented 7 years ago

@ibnesayeed Any updates in evaluating this PR?

ibnesayeed commented 7 years ago

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 commented 6 years ago

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 commented 6 years ago

Thanks @ibnesayeed! closing 😄