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

Monitor batch write #22

Closed
wants to merge 2 commits into from
Closed

Conversation

lifenod
Copy link
Contributor

@lifenod lifenod commented Dec 27, 2018

solve #13

@lifenod lifenod changed the base branch from master to fix-parse-monitor-url December 27, 2018 17:01
@lifenod lifenod requested a review from junhuif December 27, 2018 17:02
@lifenod lifenod force-pushed the monitor-batch-write branch from e85fca2 to 895f8da Compare December 27, 2018 17:03
@lifenod lifenod self-assigned this Dec 28, 2018
@lifenod lifenod removed the request for review from junhuif December 28, 2018 03:01
@lifenod lifenod force-pushed the monitor-batch-write branch from 895f8da to 3bebb6a Compare December 28, 2018 04:05
@lifenod lifenod requested a review from junhuif December 28, 2018 04:49
Copy link
Member

@junhuif junhuif left a comment

Choose a reason for hiding this comment

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

Generally fine, some problems that need to handle:

  • As we discussed that we don't merge the Fix parse monitor URL #21 , so this PR shouldn't base on the changes of Fix parse monitor URL #21.
  • We need to find a way to push the data in the queue before the go routine being cancelling, currently, the data in the queue will lost once the go routine is cancelled.

(But I don't know how to do this yet, maybe golang has a way to know about the go routine is being cancelled).

@lifenod
Copy link
Contributor Author

lifenod commented Dec 28, 2018

We need to find a way to push the data in the queue before the go routine being cancelling, currently, the data in the queue will lost once the go routine is cancelled.

May we do some cleaning work when we receive the SIGTERM signal, need to do some research.

A simple way, wait > 1m (or set a smaller batch interval) when to close the pod.

@lifenod
Copy link
Contributor Author

lifenod commented Dec 28, 2018

A simple way, wait > 1m (or set a smaller batch interval) when to close the pod.

@qi-zhou Please confirm if K8s has this waiting time.

Copy link
Member

@bodhi bodhi left a comment

Choose a reason for hiding this comment

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

I suspect using a shared array and a mutex could cause "starvation" when you have high traffic.

Only the goroutines from the HTTP requests may acquire the lock, and the goroutine to write the batched requests may never get the lock.

Remember the Go "proverb":

Do not communicate by sharing memory; instead, share memory by communicating.

(https://blog.golang.org/share-memory-by-communicating)

  1. Create an input channel for events, instead of the shared array + lock.

  2. Each request writes an event to the input channel.

  3. The batching go-routine reads events from the channel and adds them to an array.

  4. Either after the timeout or after you reach some large batch size, you can send the the batch of points to InfluxDB.

Benefits:

  • You don't need a shared array, because the buffer is private to the batching goroutine.

  • Because you don't have a shared array, you don't need the mutex.

  • It will be much easier to retry when sending data to InfluxDB fails for a 5xx reason.

  • You can control the timeout and the size of the buffer that you send to InfluxDB. Here's a refernece

@bodhi
Copy link
Member

bodhi commented Dec 28, 2018

Oops. Review submitted early by accident, sorry!

Here's a refernece

What I was going to say was: Here's an (older) reference that says 5-10k events per request is a good amount: https://community.influxdata.com/t/what-is-the-highest-performance-method-of-getting-data-in-out-of-influxdb/464

@lifenod
Copy link
Contributor Author

lifenod commented Jan 7, 2019

move to #25

@lifenod lifenod closed this Jan 7, 2019
@lifenod lifenod deleted the monitor-batch-write branch January 7, 2019 06:34
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