TableClient for each PeriodConfig in Schema #1446

codesome · 2019-06-05T17:20:30Z

Attempt to fix #1382

This PR creates a TableClient for all the past configs and deletes tables from all the clients in SyncTables.

Signed-off-by: Ganesh Vernekar <[email protected]>

gouthamve · 2019-06-07T09:16:39Z

pkg/chunk/table_manager.go

-		return err
-	}
+	for i, client := range m.clients {
+		toCreate, toCheckThroughput, toDelete, err := m.partitionTables(ctx, client, expected)


This doesn't seem right. This would mean that every single client would create a bunch of random tables (ones that belong to other clients).

Can we pass config to calculateExpectedTables() and then use that for each of the clients?

I was thinking the same. But it is the same behavior before apparently. calculateExpectedTables parses all the configs, and creates/updates/deletes from the last client. Here I was only deleting from old client, but left the behavior of the last client unchanged.

Do you want me to change it to per client for calculateExpectedTables?

But the delete calls will fail, as they are trying to delete tables that don't exist. Yeah, changing calculateExpectedTables makes sense.

Tests failed for the change described. While I am still digging into the test failure, from past implementation my understanding is that i th client uses all config from the starting till the i th config to create tables.

I changed the implementation to use 0,1,...,i'th config for the i'th client. Tests pass for that. It that the expected behaviour then?

Doesn't sound right. Why wouldn't the i'th client just use the i'th config?

What you said sounds right. Apparently the old code used all the configs for the client. I will update the tests then.

Now I see why all the configs were parsed in the old code

cortex/pkg/chunk/table_manager.go

Lines 244 to 255 in 53438df

if i+1 < len(m.schemaCfg.Configs) {

var (

endTime = m.schemaCfg.Configs[i+1].From.Unix()

gracePeriodSecs = int64(m.cfg.CreationGracePeriod / time.Second)

maxChunkAgeSecs = int64(m.maxChunkAge / time.Second)

now = mtime.Now().Unix()

)

if now >= endTime+gracePeriodSecs+maxChunkAgeSecs {

isActive = false

}

}

The old configs are parsed and checked for grace period w.r.t new configs. Also provisions read/write accordingly if the config is active/inactive. I guess we would need parsing the old configs if we want to delete the tables from the client if tables were created because of the grace period. So 0,1,...,i'th configs are only parsed inside calculateExpectedTables for the i'th client, and not necessarily used.

Could we make a rule that grace period only applies to the latest config?

Signed-off-by: Ganesh Vernekar <[email protected]>

Instead of taking only i'th config for the i'th client, take 0,1,...,i'th PeriodConfig for i'th client in calculateExpectedTables. Signed-off-by: Ganesh Vernekar <[email protected]>

Signed-off-by: Ganesh Vernekar <[email protected]>

khaines · 2019-10-21T22:00:04Z

@gouthamve @bboreham Since you two had looked at this PR previously, can you please look at it again? I'm taking a scan through old PRs to clear out older items laying about.

khaines · 2019-10-21T22:25:11Z

pkg/chunk/table_manager.go

+			if lastClient {
+				return err
+			}
+			level.Error(util.Logger).Log("msg", "error in deleting tables from TableClient")


This log output states there was an error, but offers no context or the error itself. Can you add more details to this line in order to make it useful for troubleshooting?

👍 It would be good to print the error in this log statement

jtlisi

LGTM, I think things are good as is but I did have one suggestion above. I don't think it's mandatory for approval. Also please add an error to the log statement @khaines mentioned above.

jtlisi · 2019-10-23T20:34:12Z

pkg/cortex/modules.go

-	tableClient, err := storage.NewTableClient(lastConfig.IndexType, cfg.Storage)
-	if err != nil {
-		return err
+	var tableClients []chunk.TableClient


Is there any reason we can't generate the table and bucket clients inside the NewTableManager function? Instead of instantiating the clients and passing them as a parameter would could just pass the storage config as a parameter. I think the way things are now is probably fine. However, the columnar relationship between the schema cfg and clients makes me think it might be better to encapsulate this logic inside the function. The comment I'm referring to:

// The i'th TableClient in 'tableClients' should correspond to the i'th PeriodConfig in schemaCfg.Config. // The TableClient for the last client should not be nil. func NewTableManager(cfg TableManagerConfig, schemaCfg SchemaConfig, maxChunkAge time.Duration, tableClients []TableClient, objectClient BucketClient) (*TableManager, error) {objectClient BucketClient) (*TableManager, error) {

jtlisi · 2019-10-23T20:42:33Z

pkg/chunk/table_manager.go

+		lastClient := i == len(m.clients)-1
+		expected := m.calculateExpectedTables(i)
+		if lastClient {
+			level.Info(util.Logger).Log("msg", "synching tables", "num_expected_tables", len(expected), "expected_tables", len(expected))


What is the difference between num_expected_tables and expected_tables in these two log statements?

Ah I already opened a PR to fix it upstream before looking at this comment #1754, they are the same. Noticed this log when running Loki.

Will fix it here too.

jtlisi · 2019-10-23T20:49:50Z

pkg/chunk/table_manager.go

+	startIdx := idx
+	if idx == len(m.schemaCfg.Configs)-1 {
+		// For the last client, collect tables from all the configs.
+		startIdx = 0


Why are we collecting all of the tables for the final config? Won't this lead to tables being created that will never be used?

I took a second look and had a few questions

pracucci · 2020-02-03T10:28:37Z

@gouthamve @jtlisi @bboreham What's the state of this PR? Is it something we wanna put some effort to reach a mergeable state, or should we close it?

stale · 2020-04-03T10:40:23Z

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

codesome · 2020-09-23T13:04:24Z

Closing this to let someone else take it up. I don't plan to work on this further at the moment.

TableClient for each PeriodConfig in Schema

9a86dc8

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome force-pushed the multiple-table-client branch from ba86f9c to 9a86dc8 Compare June 5, 2019 17:46

codesome marked this pull request as ready for review June 7, 2019 08:58

gouthamve reviewed Jun 7, 2019

View reviewed changes

Ganesh Vernekar added 3 commits June 7, 2019 22:33

calculateExpectedTables per config

a941e0c

Signed-off-by: Ganesh Vernekar <[email protected]>

Fix calculateExpectedTables

b6799c0

Instead of taking only i'th config for the i'th client, take 0,1,...,i'th PeriodConfig for i'th client in calculateExpectedTables. Signed-off-by: Ganesh Vernekar <[email protected]>

Fix review comments

3874bfe

Signed-off-by: Ganesh Vernekar <[email protected]>

khaines reviewed Oct 21, 2019

View reviewed changes

gouthamve requested a review from jtlisi October 23, 2019 15:53

jtlisi previously approved these changes Oct 23, 2019

View reviewed changes

jtlisi reviewed Oct 23, 2019

View reviewed changes

sandeepsukhani mentioned this pull request Mar 11, 2020

add support for bigtable as index store for integration tests #2249

Merged

stale bot added the stale label Apr 3, 2020

pracucci added keepalive Skipped by stale bot and removed stale labels Apr 3, 2020

codesome closed this Sep 23, 2020

	if i+1 < len(m.schemaCfg.Configs) {
	var (
	endTime = m.schemaCfg.Configs[i+1].From.Unix()
	gracePeriodSecs = int64(m.cfg.CreationGracePeriod / time.Second)
	maxChunkAgeSecs = int64(m.maxChunkAge / time.Second)
	now = mtime.Now().Unix()
	)
	if now >= endTime+gracePeriodSecs+maxChunkAgeSecs {
	isActive = false

	}
	}

TableClient for each PeriodConfig in Schema #1446

TableClient for each PeriodConfig in Schema #1446

Uh oh!

Conversation

codesome commented Jun 5, 2019

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!

khaines commented Oct 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtlisi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jtlisi Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci commented Feb 3, 2020

Uh oh!

stale bot commented Apr 3, 2020

Uh oh!

codesome commented Sep 23, 2020

Uh oh!

Uh oh!

jtlisi left a comment •

edited

Loading

jtlisi Oct 23, 2019 •

edited

Loading