Skip to content

service: simplify Logs() & improve docs#888

Merged
NicolasMahe merged 4 commits intodevfrom
refactoring/service/simplify-logs
Apr 19, 2019
Merged

service: simplify Logs() & improve docs#888
NicolasMahe merged 4 commits intodevfrom
refactoring/service/simplify-logs

Conversation

@ilgooz
Copy link
Copy Markdown
Contributor

@ilgooz ilgooz commented Apr 17, 2019

No description provided.

@ilgooz ilgooz force-pushed the refactoring/service/simplify-logs branch from a9042da to 1032b2f Compare April 17, 2019 09:53
* moved Dependency.Logs() logic into Service.Logs().
* removed unneded TestDependencyLogs() test.
* simplify logs tests.
@ilgooz ilgooz force-pushed the refactoring/service/simplify-logs branch from 1032b2f to d116b6c Compare April 17, 2019 09:57
krhubert
krhubert previously approved these changes Apr 17, 2019
antho1404
antho1404 previously approved these changes Apr 17, 2019
Comment thread service/logs.go Outdated
@ilgooz ilgooz dismissed stale reviews from antho1404 and krhubert via 38c25eb April 18, 2019 07:33
@NicolasMahe
Copy link
Copy Markdown
Member

I have a question that is related to logs but not blocking for this PR.

https://github.com/mesg-foundation/core/blob/1ecd99db1c8c90a60231d77f8e857309c45ddf35/service/logs.go#L23-L58

Seeing this function, I don't see anything that will make the core to 'stop' listening the logs when let's say the user stops the command service logs. I think the go routine will continue to live forever..
Am I right or there is a mechanism that will stop the stream of logs?

@ilgooz
Copy link
Copy Markdown
Contributor Author

ilgooz commented Apr 18, 2019

@NicolasMahe goroutine will eventually be closed when these writers closed inside grpc core pkg by the api cancellation.

but we should definitely improve it later to close the reader as well.

-i'm actually thinking about creating a custom io.Closer to simplify the process. and, not relevent with this but I also plan to do even more and improve the chunker pkg to implement custom io.Writers/Readers and simplify log streams all once. these are some future refactoring plans.

because for now, stdcopy will only be able to cancel after reading from the reader and trying to write to the closed writers(writers closed when their readers are closed -io.Pipe()-). but we can make it cancel sooner by closing the reader as well so it doesn't need to wait data to appear in the reader.

@NicolasMahe NicolasMahe merged commit 7c7fcc2 into dev Apr 19, 2019
@NicolasMahe NicolasMahe deleted the refactoring/service/simplify-logs branch April 19, 2019 09:25
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.

4 participants