flush boltdb files to object stores #1533

sandeepsukhani · 2019-07-26T14:13:47Z

This can be useful for running cortex using just boltdb and gcs/s3.
Few details about implementation:

Files are stored in a folder per periodic table and are named after ingester and startup timestamp so that integers can keep pushing latest changes by overriding same files
Flushes files every 15 mins and before shutting down to make index available to other services
New stores can be implemented easily by implementing methods required by store interface
Ingester to also query store when using boltdb. Limits query to only last 30 mins when flushing boltdb to store is enabled.
Queriers would keep syncing new and updated index files in a local folder which would be cleaned up by default every 24 hours.

pracucci · 2020-01-24T16:25:42Z

@sandeepsukhani Could you rebase master, fix the linter and tests before I start the review, please?

files are stored in folder per periodic table and are named after ingester flushed every 15 mins to make index available to other services files are also flushed before ingester stops to avoid any data loss new stores can be implemented easily ingester to also query store when using boltdb Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

…re to come Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

… or not Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

pracucci

I did a relatively quick first pass. I would have several minor comments which I've skipped so far, so that we can focus on the most important things first. My main concern is about the safety of changing BoltDB files on a already opened BoltDB. I assume you've already investigated this and it's safe, but I can't find any detailed comment about it in the code, so I would like to better understand how we can safely change files under the hood to BoltDB while still working on it (ie. querying it).

pracucci · 2020-01-27T16:44:39Z

pkg/chunk/local/archiver.go

+
+// ArchiveStoreClient define all the methods that a store needs to implement for managing objects
+type ArchiveStoreClient interface {
+	GetObject(ctx context.Context, objectName string) ([]byte, error)


The variable objectName would be more correct to be named objectKey because it's actually used as the object key (made of prefix + object name). I would suggest this renaming across the change set (both interface and implementations).

Same comment applies to PutObject() too.

pracucci · 2020-01-27T16:54:13Z

pkg/chunk/local/archiver.go

+
+type StoreConfig struct {
+	Store            string            `yaml:"store"`
+	AWSStorageConfig aws.StorageConfig `yaml:"aws"`


It's unfortunate we have to use aws.StorageConfig because it also brings a bunch of config options related to DynamoDB we don't really need. In the aws package we can:

Introduce S3Config (like we have GCSConfig)

Move S3, BucketNames and S3ForcePathStyle to S3Config

Inline embed S3Config into StorageConfig so that we don't introduce a breaking change (you can do it with S3 S3Config `yaml:"inline"` )

Change NewS3ObjectClient to pick S3Config in input, instead of StorageConfig

Remember to re-run make doc so that we see the updated config.

pracucci · 2020-01-27T17:05:57Z

pkg/chunk/aws/s3_storage_client.go

+
+// Put object into the store
+func (a S3ObjectClient) PutObject(ctx context.Context, objectName string, object []byte) error {
+	return instrument.CollectedRequest(ctx, "S3.Put", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {


S3.Put > S3.PutObject

pracucci · 2020-01-27T17:09:46Z

pkg/chunk/local/archiver.go

+}
+
+// ArchiveStoreClient define all the methods that a store needs to implement for managing objects
+type ArchiveStoreClient interface {


The GetObject() and PutObject() work with []byte. This means we need to load the entire index in-memory while uploading/downloading. What's the expected index size for a large cluster in production?

I'm asking because this could be an issue, and we may want to actually work with io.Reader interfaces (ie. see Thanos S3 objstore client)

pracucci · 2020-01-27T17:19:36Z

pkg/chunk/local/boltdb_index_client.go

+		return aDB, nil
+	}
+
+	b.archivedDbsMtx.Lock()


This lock blocks any index query, even of DBs different than name, until this function returns, due to archiver.Sync() and openBoltdbFile(). Have you thought about this?

pracucci · 2020-01-27T17:58:38Z

pkg/chunk/local/archiver.go

+	return nil
+}
+
+func (a *Archiver) syncTable(ctx context.Context, tableName string, sendUpdates bool) error {


In this function we do add, update and delete files for an open BoltDB table. Is it a safe operation? Is it safe doing it while, in the meanwhile, we may query it?

pracucci · 2020-01-27T18:01:22Z

pkg/chunk/local/archiver.go

+	return nil
+}
+
+func (a *Archiver) syncTable(ctx context.Context, tableName string, sendUpdates bool) error {


Do we really need this sendUpdates instead of always doing it?

pracucci · 2020-01-27T18:05:50Z

pkg/ingester/ingester.go

 		result.Timeseries = append(result.Timeseries, ts)
 		return nil
 	}, nil, 0)
-	i.metrics.queriedSeries.Observe(float64(numSeries))
+
+	if i.cfg.QueryStore {


Could you explain me why we do want all this logic, please?

pracucci · 2020-01-27T18:14:47Z

pkg/chunk/local/archiver.go

+			return err
+		}
+
+		buf, err := ioutil.ReadAll(f)


We're reading a file while BoltDB may simultaneously write on it. How can we guarantee the uploaded file is not corrupted?

pracucci · 2020-01-27T18:18:08Z

pkg/chunk/local/archiver.go

+
+	// We would use ingester name and startup timestamp for naming files while uploading so that
+	// ingester does not override old files when using same id
+	ingesterNameAndStartUpTs string


We may rename it to something like ingesterUniqueID (the fact it's built upon ingester name + timestamp is an implementation detail, but what we need to store in the variable is a unique ID for the ingester).

sandeepsukhani · 2020-02-11T06:15:42Z

Since Cortex now has TSDB support, this is not needed.
Support for custom index clients is added in #2049 for this to be implemented in Loki instead.

tomwilkie requested a review from gouthamve July 31, 2019 12:36

sandeepsukhani force-pushed the flush-boltdb-to-gcs branch from b8c1f10 to 62ee90a Compare August 20, 2019 09:56

sandeepsukhani force-pushed the flush-boltdb-to-gcs branch 3 times, most recently from e6b0f42 to 9bd6667 Compare September 17, 2019 10:30

sandeepsukhani marked this pull request as ready for review September 17, 2019 11:05

sandeepsukhani changed the title ~~Draft: Flush boltdb files to gcs~~ flush boltdb files to gcs Sep 18, 2019

sandeepsukhani force-pushed the flush-boltdb-to-gcs branch from 4a33d2f to 42725ee Compare December 5, 2019 14:34

sandeepsukhani changed the title ~~flush boltdb files to gcs~~ flush boltdb files to object stores Jan 14, 2020

sandeepsukhani force-pushed the flush-boltdb-to-gcs branch from c667894 to ac777ef Compare January 23, 2020 14:27

sandeepsukhani added 6 commits January 25, 2020 20:01

Added local store for archiver to help with testing, added a test, mo…

989b8dc

…re to come Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

added some more tests and some code refactoring

f135fbd

Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

added s3 implementation for object store in archiver

229fae8

Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

deciding based on db operation whether we want to create a new boltdb…

260de62

… or not Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

fixed broken test

afee846

Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

sandeepsukhani force-pushed the flush-boltdb-to-gcs branch from 5bb8140 to afee846 Compare January 25, 2020 15:06

sandeepsukhani added 3 commits January 26, 2020 11:10

fixed lint errors

d7dee7b

Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

refactored code to reuse most of existing object store code

8cf3d9d

Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

updated docs

c8d8244

Signed-off-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

sandeepsukhani requested a review from pracucci January 27, 2020 08:33

pracucci reviewed Jan 27, 2020

View reviewed changes

sandeepsukhani mentioned this pull request Jan 29, 2020

support for registering custom index clients, added new methods to object stores #2049

Merged

sandeepsukhani closed this Feb 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

flush boltdb files to object stores #1533

flush boltdb files to object stores #1533

Uh oh!

sandeepsukhani commented Jul 26, 2019 •

edited

Loading

Uh oh!

pracucci commented Jan 24, 2020

Uh oh!

pracucci left a comment

Uh oh!

pracucci Jan 27, 2020

Uh oh!

pracucci Jan 27, 2020

Uh oh!

pracucci Jan 27, 2020

Uh oh!

pracucci Jan 27, 2020

Uh oh!

pracucci Jan 27, 2020

Uh oh!

pracucci Jan 27, 2020

Uh oh!

pracucci Jan 27, 2020

Uh oh!

pracucci Jan 27, 2020

Uh oh!

pracucci Jan 27, 2020

Uh oh!

pracucci Jan 27, 2020

Uh oh!

sandeepsukhani commented Feb 11, 2020

Uh oh!

Uh oh!

flush boltdb files to object stores #1533

flush boltdb files to object stores #1533

Uh oh!

Conversation

sandeepsukhani commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci commented Jan 24, 2020

Uh oh!

pracucci left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandeepsukhani commented Feb 11, 2020

Uh oh!

Uh oh!

sandeepsukhani commented Jul 26, 2019 •

edited

Loading