Add support for TSI shard streaming and shard size #8491

e-dard · 2017-06-14T12:28:00Z

This commit firstly ensures that a shard's size on disk is accurately
reported when using the tsi1 index, by including the on-disk size of the
tsi1 index in the calculation.

Secondly, this commit add support for shard streaming/copying when using
the tsi1 index. Prior to this, a tsi1 index would not be correctly
restored when streaming shards.

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated

benbjohnson

Minor comments. Otherwise lgtm.

benbjohnson · 2017-06-14T12:35:40Z

CHANGELOG.md

 #### `[continuous_queries]` Section

 * `query-stats-enabled` was added with a default of `false`. When set to `true`, continuous query execution statistics are written to the default monitor store.
+=======
+  the configured maximum size, a `413 Request Entity Too Large` HTTP response is returned.
+>>>>>>> Add support for TSI shard streaming and shard size


Merge conflict?

benbjohnson · 2017-06-14T12:36:56Z

tsdb/engine/tsm1/engine.go

@@ -666,6 +662,8 @@ func (e *Engine) overlay(r io.Reader, basePath string, asNew bool) error {
 			return nil, err
 		}

+		// The filestore will only handle tsm files. Other file types will be
+		// ignored.


Why must you leave orphaned words in comment lines!?!? 😡

Your rage brings me so much joy ❤️ .

benbjohnson · 2017-06-14T12:39:11Z

tsdb/engine/tsm1/file_store_test.go

@@ -2253,9 +2253,9 @@ func TestFileStore_Stats(t *testing.T) {
 		"mem": []tsm1.Value{tsm1.NewValue(0, 1.0)},
 	})

-	replacement := files[2] + "-foo" + ".tmp" // Assumes new files have a .tmp extension
+	replacement := fmt.Sprintf("%s.%s.%s", files[2], tsm1.TmpTSMFileExtension, tsm1.TSMFileExtension) // Assumes new files have a .tmp extension


Should Tmp extension be at the end?

@benbjohnson I changed the implementation to check for either .tsm.tmp or .tsm. The previous test was creating a filename ending in -foo.tmp, which didn't match the new filters, so I updated the test to come up with a new filename that matched one of the filters.

jwilder · 2017-06-21T15:45:24Z

tsdb/engine/tsm1/engine.go

+		e.mu.RUnlock()
+		return "", err
+	}
+	e.mu.RUnlock()


I think the engine should still be locked while the index is snapshotted. If it's unlocked, it's possible the index and tsm files could be inconsistent if deletes and writes were running.

Looks like this was a pre-existing bug in that case. I'll add the lock protection.

jwilder · 2017-08-23T16:55:29Z

CHANGELOG.md

@@ -107,7 +108,11 @@ The following new configuration options are available.
 * `max-body-size` was added with a default of 25,000,000, but can be disabled by setting it to 0.
  Specifies the maximum size (in bytes) of a client request body. When a client sends data that exceeds
  the configured maximum size, a `413 Request Entity Too Large` HTTP response is returned.
+<<<<<<< HEAD


Bad merge...

jwilder · 2017-08-23T16:57:26Z

tsdb/engine/tsm1/engine.go

@@ -707,15 +705,27 @@ func (e *Engine) overlay(r io.Reader, basePath string, asNew bool) error {
 		return err
 	}

+	if idx, ok := e.index.(*tsi1.Index); ok {


I think this may create a race now since it's accessing the index w/o a RLock.

There may be other accesses to e.index that need to be protected now too.

e.index is still accessed outside of a RLock.

e-dard · 2017-10-20T12:27:10Z

@jwilder is this good to go now?

@benbjohnson can you check 4d14ff058181bda032e9fbed702f4255381be099. I didn't include these fields originally and as I was rebasing it occurred to me that we probably need to set them.

jwilder · 2017-10-20T15:25:22Z

tsdb/engine/tsm1/file_store.go

 	for _, fi := range tmpfiles {
-		if fi.IsDir() && strings.HasSuffix(fi.Name(), ".tmp") {
+		if fi.IsDir() && strings.HasSuffix(fi.Name(), ext) {


This is missing the "." now.

See a couple of lines up

jwilder · 2017-10-20T15:25:52Z

tsdb/engine/tsm1/file_store.go


 	// Rename all the new files to make them live on restart
 	for _, file := range newFiles {
 		var newName = file
-		if strings.HasSuffix(file, ".tmp") {
+		if strings.HasSuffix(file, tsmTmpExt) {


"." is missing as well.

defined on 574

jwilder · 2017-10-20T15:29:37Z

tsdb/index/tsi1/index.go

+func (i *Index) DiskSizeBytes() int64 {
+	fs := i.RetainFileSet()
+	defer fs.Release()
+	return fs.Size()


Shard.DiskSize is called fairly frequently. It created a lot of lock contention in tsm and I suspect it will cause a problem here as well. I ended up switching the sizes in tsm to be atomic counters to fix it.

@jwilder this calls down to here on each file in the fileset. It's should be cheap.

jwilder · 2017-10-20T15:30:28Z

tsdb/engine/tsm1/engine.go

@@ -707,15 +705,27 @@ func (e *Engine) overlay(r io.Reader, basePath string, asNew bool) error {
 		return err
 	}

+	if idx, ok := e.index.(*tsi1.Index); ok {


e.index is still accessed outside of a RLock.

e-dard · 2017-10-20T16:42:00Z

@jwilder see c61f533

benbjohnson · 2017-10-23T16:40:41Z

@e-dard 👍 on 4d14ff0.

This commit firstly ensures that a shard's size on disk is accurately reported when using the tsi1 index, by including the on-disk size of the tsi1 index in the calculation. Secondly, this commit add support for shard streaming/copying when using the tsi1 index. Prior to this, a tsi1 index would not be correctly restored when streaming shards.

jwilder · 2017-11-29T14:25:09Z

tsdb/engine/tsm1/file_store.go

 			// The new TSM files have a tmp extension.  First rename them.
 			newName = file[:len(file)-4]
 			if err := os.Rename(file, newName); err != nil {
 				return err
 			}
+		} else if !strings.HasSuffix(file, TSMFileExtension) {


I think this should be TmpTSMFileExtension. New files passed in are always tmp one.

That check happens in the if above. It checks for tsmTmpExt which is .tsm.tmp. This else if skips any non-tsm or non-tmp-tsm files (such as index files).

In the if above we rename the tmp tsm file and drop the .tmp. Then below this else if we process the tsm file.

Without this else if to skip non-tsm files I believe we could get caught up on tsi index files.

e-dard added area/backup and restore area/tsi labels Jun 14, 2017

e-dard requested review from benbjohnson and jwilder June 14, 2017 12:28

e-dard added the review label Jun 14, 2017

e-dard force-pushed the er-tsi-restore branch 2 times, most recently from 6aa05c8 to 6d1f805 Compare June 14, 2017 12:29

benbjohnson approved these changes Jun 14, 2017

View reviewed changes

e-dard force-pushed the er-tsi-restore branch 4 times, most recently from b253ed8 to 93e35f1 Compare June 14, 2017 16:53

jwilder suggested changes Jun 21, 2017

View reviewed changes

e-dard force-pushed the er-tsi-restore branch 2 times, most recently from 9a73263 to 04901fe Compare August 23, 2017 16:54

jwilder suggested changes Aug 23, 2017

View reviewed changes

e-dard force-pushed the er-tsi-restore branch from 340045f to 4d14ff0 Compare October 20, 2017 12:07

ghost assigned e-dard Oct 20, 2017

jwilder suggested changes Oct 20, 2017

View reviewed changes

e-dard force-pushed the er-tsi-restore branch from 3a257c1 to c61f533 Compare October 20, 2017 16:38

jwilder approved these changes Oct 20, 2017

View reviewed changes

e-dard force-pushed the er-tsi-restore branch from 8dce31a to cbd91cd Compare November 22, 2017 20:26

e-dard added 5 commits November 28, 2017 15:57

Update test for Windows

67c67ae

Fix test due to index changes

368420c

Ensure all index fields set

abae36f

Allow concurrent access to Engine Index

38e0dd6

e-dard added 2 commits November 28, 2017 15:57

Ensure index can track fields

041a383

Fix rebase

b10249a

e-dard force-pushed the er-tsi-restore branch from cbd91cd to b10249a Compare November 28, 2017 15:58

Refactor based on new design

81976bc

jwilder reviewed Nov 29, 2017

View reviewed changes

jwilder approved these changes Nov 29, 2017

View reviewed changes

e-dard merged commit c2f7f0f into master Nov 29, 2017

e-dard deleted the er-tsi-restore branch November 29, 2017 15:40

ghost removed the review label Nov 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TSI shard streaming and shard size #8491

Add support for TSI shard streaming and shard size #8491

e-dard commented Jun 14, 2017 •

edited

Loading

benbjohnson left a comment

benbjohnson Jun 14, 2017

benbjohnson Jun 14, 2017

e-dard Jun 14, 2017

benbjohnson Jun 14, 2017

e-dard Jun 14, 2017

jwilder Jun 21, 2017

e-dard Aug 23, 2017

jwilder Aug 23, 2017

jwilder Aug 23, 2017

jwilder Aug 23, 2017

jwilder Oct 20, 2017

e-dard commented Oct 20, 2017

jwilder Oct 20, 2017

e-dard Oct 20, 2017

jwilder Oct 20, 2017

e-dard Oct 20, 2017

jwilder Oct 20, 2017

e-dard Oct 20, 2017

jwilder Oct 20, 2017

e-dard commented Oct 20, 2017

benbjohnson commented Oct 23, 2017

jwilder Nov 29, 2017

e-dard Nov 29, 2017

Add support for TSI shard streaming and shard size #8491

Add support for TSI shard streaming and shard size #8491

Conversation

e-dard commented Jun 14, 2017 • edited Loading

Required for all non-trivial PRs

benbjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-dard commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-dard commented Oct 20, 2017

benbjohnson commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-dard commented Jun 14, 2017 •

edited

Loading