Remote Write Checkpointing by njingco · Pull Request #7710 · prometheus/prometheus

njingco · 2020-07-31T21:46:13Z

Currently remote write sends data via Prometheus WAL, but the pointer for each remote write queue in the WAL does not persist across restarts, as per #6333 . This PR will attempt to solve this issue by recording an identifier to indicate which segment file to re transmit on Prometheus restart. Re transmitting the whole segment file will potentially have zero data loss. Please refer to my proposal document for more information: Proposal Doc

This PR is currently in progress, but will include the base of the proposal. It will include recording the identifier during shut down, and at a set time, identifying the segment number for each endpoint, and error/corruption checks for the record json file.

It currently has the ability to record the identifier into a json. Sample of the record file is in SegmentRecord.json. It also does corruption checks of the json file at the start of the queue_manager.go.

Whats left to do in this PR add the timed recording from start of queue_manager, and implementing the time customization in the configuration file.

This PR will not have the implementation of setting the segment file for re transmission. This will be in a future PR. This PR will only manage the recording of the identifiers.

…te the segment record to a json file Signed-off-by: njingco <jingco.nicole@gmail.com>

Signed-off-by: njingco <jingco.nicole@gmail.com>

…e, updated the file as well Signed-off-by: njingco <jingco.nicole@gmail.com>

…segment records from multiple endpoints Signed-off-by: njingco <jingco.nicole@gmail.com>

Signed-off-by: njingco <jingco.nicole@gmail.com>

cstyan

Good progress, made some comments. Let me know if anything is unclear.

storage/remote/queue_manager.go

tsdb/wal/watcher.go

SegmentRecord.json

storage/remote/queue_manager.go

csmarchbanks · 2020-08-06T22:46:20Z

tsdb/wal/watcher.go

 		}

+		// Sets the current Segment files name
+		w.SegmentFile = SegmentName("", currentSegment)


Would it be possible for the watcher to have progressed to a new segment, without sending all samples from the previous segment? I think if you had a large enough buffer capacity in the queues that it could happen.

Yes it is possible

storage/remote/queue_manager.go

Signed-off-by: njingco <jingco.nicole@gmail.com>

storage/remote/queue_manager.go

tsdb/wal/watcher.go

…tly update the json file every set time, only made queue_manager have an update function for the CheckpointRecord Signed-off-by: njingco <jingco.nicole@gmail.com>

Signed-off-by: njingco <jingco.nicole@gmail.com>

…o master

Signed-off-by: njingco <jingco.nicole@gmail.com>

cstyan

made some comments, good progress

storage/remote/queue_manager.go

storage/remote/write.go

cstyan · 2020-09-08T18:34:45Z

tsdb/wal/watcher.go

 		w.recordsReadMetric.WithLabelValues(recordType(dec.Type(rec))).Inc()

+		// Sets the current Segment files it's reading from
+		w.CurrSegmentFile = SegmentName("", segmentNum)


this should be set in the same location as the metric is being set

would this not go in the readSegment() since this is where the current segment is being read

It's cleaner to set it where the control of what currentSegment is set to is. The Watchers Run function controls the local currentSegment, both passing the value to watch (and subsequently readSegment) and updating the value to the next segment number when watch returns. It's also easier for the next person who comes along to read the code if these two related variables are set in the same location.

…reating the checkpoint file, added the context the timed checkpointing go thread Signed-off-by: njingco <jingco.nicole@gmail.com>

cstyan · 2020-09-28T21:55:18Z

storage/remote/queue_manager.go


+// UpdateEndpointRecord gets the current segment number read from the watcher and stores it to
+func (t *QueueManager) UpdateEndpointRecord() {
+	t.endpointRecord.Segment = t.watcher.CurrSegmentFile


watcher should maybe have a CurrentSegment() function rather than exposing one of it's fields

cstyan · 2020-09-28T21:55:30Z

tsdb/wal/watcher.go

 		w.recordsReadMetric.WithLabelValues(recordType(dec.Type(rec))).Inc()

+		// Sets the current Segment files it's reading from
+		w.CurrSegmentFile = SegmentName("", segmentNum)


It's cleaner to set it where the control of what currentSegment is set to is. The Watchers Run function controls the local currentSegment, both passing the value to watch (and subsequently readSegment) and updating the value to the next segment number when watch returns. It's also easier for the next person who comes along to read the code if these two related variables are set in the same location.

cstyan · 2020-09-28T21:56:02Z

tsdb/wal/watcher.go


-		MaxSegment: -1,
+		MaxSegment:      -1,
+		CurrSegmentFile: "",


empty string is the default value for a string, no need to set it here

cstyan · 2020-09-28T21:57:28Z

tsdb/wal/wal.go

 // before.
-var castagnoliTable = crc32.MakeTable(crc32.Castagnoli)
+var (
+	castagnoliTable = crc32.MakeTable(crc32.Castagnoli)


are you using this for something, maybe forgot to add a file to a commit?

cstyan · 2020-09-28T22:00:07Z

storage/remote/queue_manager.go

+
+// EndpointRecord Structure holds the segment the endpoint url
+type EndpointRecord struct {
+	Segment  string


the fields on this and checkpoint record don't need to be exported if you don't have a need to access those fields directly outside of the storage/remote package.

cstyan · 2020-09-28T22:10:37Z

storage/remote/queue_manager.go

+		}
+
+		// Segmentnumber out of bounds
+		if segmentNumber < wal.MinSegmentNumber || segmentNumber > wal.MaxSegmentNumber {


We should maybe have the watcher have a ValidSegment function that uses wal.Segments to check if segmentNumber exists in the current WAL directory. Thoughts @csmarchbanks ?

storage/remote to cache these segment offsets to disk. Deprecates prometheus#7710. Deprecates prometheus#8918. Closes prometheus#8809. Signed-off-by: Robert Fratto <robertfratto@gmail.com>

roidelapluie · 2023-09-26T11:22:40Z

We have looked at this pull request during our bug scrub.

@csmarchbanks @cstyan, we don't have much context on our side. What is the status of this pull request? is this still valid/needed?

Thank you for your contribution anyway @njingco

cstyan · 2023-10-18T03:29:46Z

We can close this implementation. We'd still like this feature but no one has worked on this implementation path in quite some time.

njingco added 8 commits July 28, 2020 03:43

Added SegmentFile data in the watcher structure, made function to wri…

5158f66

…te the segment record to a json file Signed-off-by: njingco <jingco.nicole@gmail.com>

cleaned up code

38a16f6

Signed-off-by: njingco <jingco.nicole@gmail.com>

add writing the remote write url to the remote SegmentRecord.json fil…

dc1b5d6

…e, updated the file as well Signed-off-by: njingco <jingco.nicole@gmail.com>

Added json error detection and handeling, added functionality to add …

cb58494

…segment records from multiple endpoints Signed-off-by: njingco <jingco.nicole@gmail.com>

sync branch

c0ed046

Signed-off-by: njingco <jingco.nicole@gmail.com>

Merge branch 'master' into remote-write-segment

b0ba067

change the port number back to :1234

c76751b

Signed-off-by: njingco <jingco.nicole@gmail.com>

removed offset element in the SegmentRecord.json

1334112

Signed-off-by: njingco <jingco.nicole@gmail.com>

roidelapluie requested review from bwplotka, csmarchbanks and cstyan August 1, 2020 06:38

cstyan reviewed Aug 5, 2020

View reviewed changes

csmarchbanks reviewed Aug 6, 2020

View reviewed changes

added changes from PR review by Callum and Chris

8c42feb

Signed-off-by: njingco <jingco.nicole@gmail.com>

cstyan reviewed Aug 11, 2020

View reviewed changes

storage/remote/queue_manager.go Outdated Show resolved Hide resolved

storage/remote/queue_manager.go Outdated Show resolved Hide resolved

storage/remote/queue_manager.go Outdated Show resolved Hide resolved

tsdb/wal/watcher.go Outdated Show resolved Hide resolved

njingco added 4 commits September 2, 2020 23:18

moved json writing function to write.go, and added a timer to constan…

810307e

…tly update the json file every set time, only made queue_manager have an update function for the CheckpointRecord Signed-off-by: njingco <jingco.nicole@gmail.com>

moved the segment max and min variables to wal as per callums comment

557fd0a

Signed-off-by: njingco <jingco.nicole@gmail.com>

Merge branch 'master' of https://github.com/prometheus/prometheus int…

4affd00

…o master

updating branch, fixed some string error messages

a3bcb1b

Signed-off-by: njingco <jingco.nicole@gmail.com>

cstyan reviewed Sep 8, 2020

View reviewed changes

cstyan mentioned this pull request Sep 16, 2020

WAL replay remote storage tailing error message after lagging behind #7940

Closed

added mutex to current Segment number, added the temporary file for c…

db9ae3a

…reating the checkpoint file, added the context the timed checkpointing go thread Signed-off-by: njingco <jingco.nicole@gmail.com>

cstyan reviewed Sep 28, 2020

View reviewed changes

stale bot added the stale label Nov 28, 2020

Base automatically changed from master to main February 23, 2021 19:36

stale bot removed the stale label Feb 23, 2021

stale bot added the stale label Apr 24, 2021

cstyan mentioned this pull request May 10, 2021

[Remote Write] WAL watching best-effort start from a checkpoint instead of time.Now #8809

Open

rfratto mentioned this pull request Nov 24, 2021

tsdb/wal: add support for saving and restoring read segments #9862

Closed

rfratto mentioned this pull request Sep 6, 2022

Disable WAL for agent mode #10750

Open

cstyan closed this Oct 18, 2023

Conversation

njingco commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cstyan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cstyan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roidelapluie commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cstyan commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

njingco commented Jul 31, 2020 •

edited

Loading

roidelapluie commented Sep 26, 2023 •

edited

Loading