Skip to content
This repository has been archived by the owner on Aug 30, 2019. It is now read-only.

writer: simplify monitor #503

Merged
merged 2 commits into from
Oct 24, 2018
Merged

writer: simplify monitor #503

merged 2 commits into from
Oct 24, 2018

Conversation

gbbr
Copy link
Contributor

@gbbr gbbr commented Oct 24, 2018

In this change:

  • Move backoff package into writer folder.
  • Use only one struct. for events with a type field to avoid reflection.
  • Slightly minimise memory copying.

@gbbr gbbr added this to the 6.7.0 milestone Oct 24, 2018
@gbbr gbbr requested a review from AlexJF October 24, 2018 08:34
@@ -65,7 +65,7 @@ func (w *multiSender) Send(p *Payload) {
}
}

func (w *multiSender) Monitor() <-chan interface{} { return w.mch }
func (w *multiSender) monitor() <-chan monitorEvent { return w.mch }
Copy link

Choose a reason for hiding this comment

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

Why make Monitor private? This way, no one outside of this package will be able to use a sender?

type eventType int

const (
eventTypeSuccess eventType = iota
Copy link

Choose a reason for hiding this comment

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

Again, if these are private no one outside of this package will be able to use a sender. If this is indeed your intention we should set the whole PayloadSender and everything directly related to it as private and only leave actual writers public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the plan, yeah. Nobody's using them outside of writer, so there is no point to export them or make them visible in the public documentation. Most of these packages should be internal anyway.

Would you like me to do that within the scope of this PR?

Copy link

@AlexJF AlexJF left a comment

Choose a reason for hiding this comment

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

Ok just confirming this was indeed your intention. In that case feel free to do it over several PRs.

@gbbr
Copy link
Contributor Author

gbbr commented Oct 24, 2018

Thank you Alex.

@gbbr gbbr merged commit 61ee09a into master Oct 24, 2018
@gbbr gbbr deleted the gbbr/writer branch October 24, 2018 09:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants