Create index table from local table client for boltdb #2441

adityacs · 2020-04-10T07:08:14Z

What this PR does:
When using boltdb for index, calling create and update table doesn't have any action and this makes logs too much verbose.
Reference: grafana/loki#1786

Checklist

Tests updated

adityacs · 2020-04-10T07:52:07Z

pkg/chunk/table_manager.go

+func (m *TableManager) validateClient() bool {
+	if reflect.TypeOf(m.client).String() == "*local.tableClient" {
+		return false
+	}
+	return true
+}


If this implementation is OK, will add a small test for this. Let me know if we can do it in any other better way.

pracucci

Thanks @adityacs for submitting this PR. I understand this log could be quite noisy (even if it's an info, and thus could be suppressed via -log.level=warn). Anyway, I think this is quite an hacky solution and I think we should look for a cleaner solution. I think the main problem is that the boltdb TableClient.CreateTable() is a noop, while ListTables() look for tables as filesystem directories. Have you check if creating the expected dir in TableClient.CreateTable() does the job?

adityacs · 2020-04-10T12:34:54Z

@pracucci Thanks for the review. Totally agree this is a hacky solution. We can create the directory in CreateTable. However, there is a catch here, we configure the expected number of tables here

cortex/pkg/chunk/table_manager.go

Line 371 in 26fa432

result = append(result, config.IndexTables.periodicTables(

and we don't need all of this when we are using boltdb.

I haven't gone through the code flow completely. I may be wrong here. When I run the code I see that boltdb directory is created with just the last entry from the list of expected tables. So, we simply loop in here

cortex/pkg/chunk/table_manager.go

Line 444 in 26fa432

level.Info(util.Logger).Log("msg", "creating table", "table", desc.Name)

for tables which is not required to be created.

adityacs · 2020-04-14T08:15:46Z

@pracucci One solution I could think of is can we change this interface to support an iterator?
If we add an iterator, we can create and update tables by checking if it has nextValue. If it has nextValue, continue, otherwise we break the loop here

cortex/pkg/chunk/table_manager.go

Line 444 in 26fa432

level.Info(util.Logger).Log("msg", "creating table", "table", desc.Name)

In case of boltdb we will have only one value(in case of using schema v11) to create and after creating the expected table we can return nil as next value so that we can break the loop.

OR

Somewhere we have to check if client is local.tableClient and skip including in expectedTables. May be even here we can skip

cortex/pkg/chunk/table_manager.go

Line 371 in 26fa432

result = append(result, config.IndexTables.periodicTables(

However, this would still be just work around solution

WDYT?

pracucci · 2020-04-14T13:28:26Z

I still believe this whole change should be relegated to BoltDB only. The BoltDB CreateTable() and ListTables() should be made to play nicely together. @sandeepsukhani do you have any feedback on this?

sandeepsukhani · 2020-04-23T13:09:23Z

Hey @adityacs,
Thanks for the PR! So after discussing this internally, we have decided to create the files from TableManager for BoltDB as well i.e change the implementation of the CreateTable method in TableClient for BoltDB to create a file. I guess you need not use bbolt package to do it and create a normal file using maybe ioutil.WriteFile with empty data . You need to test it though whether not using bbolt.Open creates any problems with reads/writes in boltDB index client.

adityacs · 2020-04-24T04:28:16Z

@sandeepsukhani Thanks for the feedback. Will make the required changes.

@pracucci Sorry for the confusion I created. I was not looking into default value set in loki for the from field in SchemaConfig. So that led to my misunderstanding of expectedTables calculation.

sandeepsukhani

I see a problem with keeping the file reference open while creating the files from TableManager. Suggested changes for it.

sandeepsukhani · 2020-04-27T13:47:05Z

pkg/chunk/local/boltdb_table_client.go

+	if _, err := os.OpenFile(filepath.Join(c.directory, desc.Name), os.O_RDWR|os.O_CREATE, 0666); err != nil {
+		return err
+	}


I think this would keep the file reference open. Let us do this instead.

Suggested change

if _, err := os.OpenFile(filepath.Join(c.directory, desc.Name), os.O_RDWR|os.O_CREATE, 0666); err != nil {

return err

}

return ioutil.WriteFile(filepath.Join(c.directory, desc.Name), nil, 0666)

Perfect. Thanks @sandeepsukhani
Other way we can call Close() on the file. However, let's stick to your code. Less number of lines 😄

adityacs · 2020-04-27T14:07:52Z

@sandeepsukhani One more thing. Should we change this to Debug ?

cortex/pkg/chunk/table_manager.go

Line 499 in 9895530

    
           level.Info(util.Logger).Log("msg", "provisioned throughput on table, skipping", "table", current.Name, "read", current.ProvisionedRead, "write", current.ProvisionedWrite)

sandeepsukhani · 2020-04-27T15:40:24Z

@sandeepsukhani One more thing. Should we change this to Debug ?

cortex/pkg/chunk/table_manager.go

Line 499 in 9895530

level.Info(util.Logger).Log("msg", "provisioned throughput on table, skipping", "table", current.Name, "read", current.ProvisionedRead, "write", current.ProvisionedWrite)

Are you seeing that in the logs with the change in CreateTable method of boltdb table client?
I feel it should not but I might be wrong though.

jtlisi

LGTM

adityacs · 2020-04-28T06:41:36Z

@sandeepsukhani Yeah, I still see in the logs

sandeepsukhani · 2020-04-28T08:06:42Z

@sandeepsukhani Yeah, I still see in the logs

@adityacs that sounds strange. I tried it locally and I do not see that in my logs. Can you please push the changes to see what you have?
Also please share the table manager config that you tried with here in the comments.

adityacs · 2020-04-28T09:34:56Z

I am just using the default configs

table_manager:
  chunk_tables_provisioning:
    inactive_read_throughput: 0
    inactive_write_throughput: 0
    provisioned_read_throughput: 0
    provisioned_write_throughput: 0
  index_tables_provisioning:
    inactive_read_throughput: 0
    inactive_write_throughput: 0
    provisioned_read_throughput: 0
    provisioned_write_throughput: 0
  retention_deletes_enabled: false
  retention_period: 0s

adityacs · 2020-04-28T09:43:26Z

@sandeepsukhani I have not disabled ThroughputUpdates so updateTable is not getting skipped. Is that correct?

sandeepsukhani · 2020-04-28T10:00:38Z

@sandeepsukhani I have not disabled ThroughputUpdates so updateTable is not getting skipped. Is that correct?

The throughput settings are only relevant for dynamodb, for all the other clients it is a noop so I guess you need not worry about it in this PR. I did not have any of those throughput settings set while trying it and I was not seeing that line being logged for me. Also, those throughput settings are removed from all the default configs to let only dynamodb users set them when need be.

sandeepsukhani

LGTM

sandeepsukhani · 2020-04-28T14:22:42Z

Let us not merge this, we should have a check in CreateTable to see if the file already exists. Thanks to @pracucci for pointing this out.

sandeepsukhani · 2020-04-28T14:23:18Z

I am checking to see if we can do it atomically.

sandeepsukhani

Some minor changes required for avoiding data loss during race condition.

sandeepsukhani · 2020-04-28T15:15:09Z

pkg/chunk/local/boltdb_table_client.go

@@ -36,7 +37,7 @@ func (c *TableClient) ListTables(ctx context.Context) ([]string, error) {
 }

 func (c *TableClient) CreateTable(ctx context.Context, desc chunk.TableDesc) error {
-	return nil
+	return ioutil.WriteFile(filepath.Join(c.directory, desc.Name), nil, 0666)


Sorry @adityacs for all the back and forth. To avoid any data loss during race conditions let us change it to the following which should create the file if it does not exist and if it is there already, it should open the file in read-only mode.

Suggested change

return ioutil.WriteFile(filepath.Join(c.directory, desc.Name), nil, 0666)

fl, err := os.OpenFile(filepath.Join(c.directory, desc.Name), os.O_CREATE|os.O_RDONLY, 0666)

if err != nil {

return err

}

return fl.Close()

@sandeepsukhani Just one question before making this change.

Since, we don't write any data while we create file using ioutil.Writefile and ioutil.Writefile opens the file in WRONLY mode and also we have the bbolt.Open() in boltdb_index_client which in turn does a file lock when doing any Read/Write operation https://github.com/etcd-io/bbolt/blob/a8af23b57f672fef05637de531bba5aa00013364/db.go#L223

Would there be data race still?

I think only way to tell about it for sure is to write some tests with that scenario. If we get an error here when file is already locked then we can avoid returning an error for it because it means file is already there.

Sure. Will try to add the tests

@sandeepsukhani You will be definitely aware of this. Still, this is what I found out.
We could have not used ioutil.Writefile since it truncates the file if already existing.

Also, regarding boltdb file lock. It only does the advisory lock so any other process can be non cooperative and override the lock. I don't think we would run in to that situation here as the binary runs as single process. So, adding test for it is not required.

So, we are good with the recent change you mentioned. Have added a test to verify "trying to create an already existing file doesn't modify any content". This is not so much required to test. We can remove it.

Signed-off-by: Aditya C S <[email protected]>

sandeepsukhani

LGTM! I have a feeling that os.OpenFile does not honor/check file locks. Since we are opening the file in readonly mode it still is safe for us, without acquiring a lock. I think this is good to go.

adityacs · 2020-04-30T11:22:50Z

@sandeepsukhani Yeah os.OpenFile doesn't check file locks.

pracucci

Thanks!

pull-request-size bot added the size/S label Apr 10, 2020

adityacs force-pushed the master branch 3 times, most recently from 7aafeb0 to 971251e Compare April 10, 2020 07:50

adityacs commented Apr 10, 2020

View reviewed changes

adityacs force-pushed the master branch 4 times, most recently from 1548885 to 9cee737 Compare April 10, 2020 10:58

pracucci reviewed Apr 10, 2020

View reviewed changes

adityacs force-pushed the master branch 2 times, most recently from e801f55 to ce845b3 Compare April 25, 2020 06:13

pull-request-size bot added size/XS and removed size/S labels Apr 25, 2020

gouthamve approved these changes Apr 27, 2020

View reviewed changes

sandeepsukhani suggested changes Apr 27, 2020

View reviewed changes

jtlisi approved these changes Apr 27, 2020

View reviewed changes

adityacs force-pushed the master branch from ce845b3 to 16c5f4e Compare April 28, 2020 09:34

sandeepsukhani approved these changes Apr 28, 2020

View reviewed changes

sandeepsukhani suggested changes Apr 28, 2020

View reviewed changes

adityacs force-pushed the master branch from 16c5f4e to dcdc718 Compare April 30, 2020 09:37

pull-request-size bot added size/M and removed size/XS labels Apr 30, 2020

adityacs changed the title ~~Skip table creation and update for local table client~~ Create index table from local table client for boltdb Apr 30, 2020

Create index table from local table client for boltdb

7a7c8de

Signed-off-by: Aditya C S <[email protected]>

adityacs force-pushed the master branch from dcdc718 to 7a7c8de Compare April 30, 2020 09:52

sandeepsukhani approved these changes Apr 30, 2020

View reviewed changes

pracucci approved these changes Apr 30, 2020

View reviewed changes

pracucci merged commit 65d3b34 into cortexproject:master Apr 30, 2020

pracucci mentioned this pull request Apr 30, 2020

Fixed tests #2551

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create index table from local table client for boltdb #2441

Create index table from local table client for boltdb #2441

adityacs commented Apr 10, 2020 •

edited

Loading

adityacs Apr 10, 2020

pracucci left a comment

adityacs commented Apr 10, 2020 •

edited

Loading

adityacs commented Apr 14, 2020 •

edited

Loading

pracucci commented Apr 14, 2020

sandeepsukhani commented Apr 23, 2020

adityacs commented Apr 24, 2020 •

edited

Loading

sandeepsukhani left a comment

sandeepsukhani Apr 27, 2020

adityacs Apr 27, 2020

adityacs commented Apr 27, 2020

sandeepsukhani commented Apr 27, 2020

jtlisi left a comment

adityacs commented Apr 28, 2020

sandeepsukhani commented Apr 28, 2020

adityacs commented Apr 28, 2020

adityacs commented Apr 28, 2020

sandeepsukhani commented Apr 28, 2020

sandeepsukhani left a comment

sandeepsukhani commented Apr 28, 2020

sandeepsukhani commented Apr 28, 2020

sandeepsukhani left a comment

sandeepsukhani Apr 28, 2020

adityacs Apr 29, 2020

sandeepsukhani Apr 29, 2020

adityacs Apr 29, 2020

adityacs Apr 30, 2020

sandeepsukhani left a comment

adityacs commented Apr 30, 2020

pracucci left a comment

-	return ioutil.WriteFile(filepath.Join(c.directory, desc.Name), nil, 0666)
+	fl, err := os.OpenFile(filepath.Join(c.directory, desc.Name), os.O_CREATE|os.O_RDONLY, 0666)
+	if err != nil {
+		return err
+	}
+	return fl.Close()

Create index table from local table client for boltdb #2441

Create index table from local table client for boltdb #2441

Conversation

adityacs commented Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

adityacs commented Apr 10, 2020 • edited Loading

adityacs commented Apr 14, 2020 • edited Loading

pracucci commented Apr 14, 2020

sandeepsukhani commented Apr 23, 2020

adityacs commented Apr 24, 2020 • edited Loading

sandeepsukhani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityacs commented Apr 27, 2020

sandeepsukhani commented Apr 27, 2020

jtlisi left a comment

Choose a reason for hiding this comment

adityacs commented Apr 28, 2020

sandeepsukhani commented Apr 28, 2020

adityacs commented Apr 28, 2020

adityacs commented Apr 28, 2020

sandeepsukhani commented Apr 28, 2020

sandeepsukhani left a comment

Choose a reason for hiding this comment

sandeepsukhani commented Apr 28, 2020

sandeepsukhani commented Apr 28, 2020

sandeepsukhani 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

sandeepsukhani left a comment

Choose a reason for hiding this comment

adityacs commented Apr 30, 2020

pracucci left a comment

Choose a reason for hiding this comment

adityacs commented Apr 10, 2020 •

edited

Loading

adityacs commented Apr 10, 2020 •

edited

Loading

adityacs commented Apr 14, 2020 •

edited

Loading

adityacs commented Apr 24, 2020 •

edited

Loading