Implementation of Pilosa Index Driver #205

kuba-- · 2018-05-25T21:12:44Z

It closes: #174
Design doc.: https://docs.google.com/document/d/1rWeR739rZsSnCBsXqyviVss8JAO6msV9B1PoDD45O2A/edit?usp=sharing

kuba-- · 2018-05-26T10:38:15Z

Note

Regarding Get(key ...interface{}) (IndexLookup, error) implementation.
Right now, if nokey parameter was passed the function returns an iterator which will go through all index locations in the same order as sql.IndexKeyValueIter passed to Save returned.

ajnavarro · 2018-05-28T08:30:26Z

sql/index.go

@@ -111,9 +110,9 @@ type IndexDriver interface {
 	// Create a new index. If exprs is more than one expression, it means the
 	// index has multiple columns indexed. If it's just one, it means it may
 	// be an expression or a column.
-	Create(db, table, id string, expressionHashes []hash.Hash, config map[string]string) (Index, error)
+	Create(db, table, id string, expressionHashes []ExpressionHash, config map[string]string) (Index, error)
 	// Load the index at the given path.


Load -> LoadAll

ajnavarro · 2018-05-28T08:31:39Z

sql/index/config.go

@@ -0,0 +1,107 @@
+package index


Should we add some documentation to methods and constructors on this file?

Added extra comments. So far I think it's clear what it does.

ajnavarro · 2018-05-28T08:34:40Z

sql/index/pilosa/driver_test.go

+
+	err = d.Delete(sqlIdx)
+	require.Nil(err)
+}


Can we test what happens if the index generation is stopped abruptly?

We can add some tests for "failing cases", but generally with docker container after restart it will recreate a schema. At the end we won't find anything in bitmaps. The only functions which should work are Has and Get() (empty get) where we do not touch pilosa.
So what's your opinion?

Have some tests for failing cases sound great. I'm afraid of having an index loaded that make a query fails just because is corrupted, or an index that appears to work correctly but it has incorrect or partial results.

kuba-- · 2018-05-29T10:56:55Z

@ajnavarro - I've added PilosaHiccup test, where I restart pilosa container every second. In a meantime I try to Save/Load/Get with retry logic.

ajnavarro · 2018-06-04T09:07:40Z

sql/index/pilosa/driver.go

+
+	var indexes []sql.Index
+	err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
+		if err != nil {


should we just skip the directory? maybe that error is a permission denied error or something similar.

What we can do (except logging)?

Stop loading all indexes

Return accumulated error with all dirs which cannot be loaded

Yep, we should handle that error instead of just skipping it

erizocosmico · 2018-06-04T10:12:47Z

sql/index.go

@@ -285,7 +284,7 @@ func exprListsEqual(a, b []hash.Hash) bool {
 				continue
 			}

-			if reflect.DeepEqual(va.Sum(nil), vb.Sum(nil)) {
+			if reflect.DeepEqual(va, vb) {


I think you can just do va == vb, since they are byte slices

Hmm, check: https://play.golang.org/p/_dRDL0lErrV
I get an error: "(slice can only be compared to nil)"

Sorry, the ones you can compare are byte arrays, not slices haha. Nvm, then

erizocosmico · 2018-06-04T10:14:12Z

sql/index/config.go

+
+// WriteConfigFile writes the configuration to dir/config.yml file.
+func WriteConfigFile(dir string, cfg *Config) error {
+	path := filepath.Join(dir, ConfigFileName)


this config file is not read and written concurrently right?

should not be. Generally it's not thread safe implementation. More like z internal one.

erizocosmico · 2018-06-04T10:15:16Z

sql/index/pilosa/driver.go

+
+	var indexes []sql.Index
+	err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
+		if err != nil {


Yep, we should handle that error instead of just skipping it

erizocosmico · 2018-06-04T10:16:28Z

sql/index/pilosa/driver.go

+			}
+			err = mapping.putLocation(index.Name(), colID, location)
+		}
+	}


what happens if there is an error writing the mapping but everything is already synced with pilosa? Should we implement some kind of transaction for this to ensure data integrity?

Would be great. So far if mapping fails in Save then the function returns error.
We can cal it again and overwrite mapping and pilosa bitmap (what we can find in tests). so at the end should not be a problem just retry.

ajnavarro · 2018-06-04T10:25:10Z

Why is Travis not executed?

kuba-- · 2018-06-04T10:39:43Z

I've updated LoadAll (I accumulate errors) so at the end you can decide do you want to go only with what successfully was loaded or break/stop loading.
Regarding travis I do not know, maybe it's because I check if docker is running before test pilosa.
So I commented these checks as a potential test blockers.

kuba-- · 2018-06-04T10:42:38Z

... or maybe just restarting pilosa container in tests hangs a travis randomly (because it works especially when the traffic is low)

kuba-- · 2018-06-04T10:45:04Z

I started build manually, we'll see how it goes

kuba-- · 2018-06-04T10:47:03Z

When I started manually everything looks fine - weird.

kuba-- · 2018-06-04T11:01:18Z

Something is wrong with reporting build status. Build finished with success: https://travis-ci.org/kuba--/go-mysql-server/builds/387725498
and status url says "unknown"

kuba-- · 2018-06-04T13:30:44Z

...and build passed

erizocosmico · 2018-06-04T15:04:13Z

As a last small request, can you cleanup a little bit the history of the PR? I see a "Merge" commit in there

Signed-off-by: kuba-- <kuba@sourced.tech>

kuba-- · 2018-06-04T20:25:45Z

I've squashed this PR to 2 commits - implementation and update (pull master and resolve conflicts)

ajnavarro

LGTM!

erizocosmico · 2018-06-05T07:43:56Z

Shall we merge this @ajnavarro? I'm gonna update gitbase and it'd be nice to get this too.

kuba-- requested review from erizocosmico and ajnavarro and removed request for erizocosmico May 25, 2018 21:12

kuba-- added the enhancement New feature or request label May 25, 2018

kuba-- requested review from a team and erizocosmico and removed request for erizocosmico May 26, 2018 10:20

ajnavarro suggested changes May 28, 2018

View reviewed changes

kuba-- self-assigned this May 30, 2018

This was referenced May 30, 2018

Implement Pilosa IndexDriver #174

Closed

Implement pilosa index simple interface (Index.Get(...)) #175

Closed

Implement IndexDriver Load and Delete for pilosa index #184

Closed

ajnavarro approved these changes Jun 4, 2018

View reviewed changes

erizocosmico reviewed Jun 4, 2018

View reviewed changes

ajnavarro approved these changes Jun 4, 2018

View reviewed changes

erizocosmico approved these changes Jun 4, 2018

View reviewed changes

kuba-- added 2 commits June 4, 2018 20:58

Implement Pilosa index driver

b11fb01

Signed-off-by: kuba-- <kuba@sourced.tech>

Update from master

6c6529e

Signed-off-by: kuba-- <kuba@sourced.tech>

ajnavarro approved these changes Jun 5, 2018

View reviewed changes

ajnavarro merged commit 0b8a5d0 into src-d:master Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of Pilosa Index Driver #205

Implementation of Pilosa Index Driver #205

kuba-- commented May 25, 2018

kuba-- commented May 26, 2018 •

edited

Loading

ajnavarro May 28, 2018

kuba-- May 28, 2018

ajnavarro May 28, 2018

kuba-- May 28, 2018

ajnavarro May 28, 2018

kuba-- May 28, 2018

ajnavarro May 28, 2018

kuba-- commented May 29, 2018

ajnavarro Jun 4, 2018

kuba-- Jun 4, 2018

erizocosmico Jun 4, 2018

erizocosmico Jun 4, 2018

kuba-- Jun 4, 2018

erizocosmico Jun 4, 2018

erizocosmico Jun 4, 2018

kuba-- Jun 4, 2018

erizocosmico Jun 4, 2018

erizocosmico Jun 4, 2018

kuba-- Jun 4, 2018

ajnavarro commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

erizocosmico commented Jun 4, 2018

kuba-- commented Jun 4, 2018

ajnavarro left a comment

erizocosmico commented Jun 5, 2018

Implementation of Pilosa Index Driver #205

Implementation of Pilosa Index Driver #205

Conversation

kuba-- commented May 25, 2018

kuba-- commented May 26, 2018 • edited Loading

Note

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

kuba-- commented May 29, 2018

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

ajnavarro commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

kuba-- commented Jun 4, 2018

erizocosmico commented Jun 4, 2018

kuba-- commented Jun 4, 2018

ajnavarro left a comment

Choose a reason for hiding this comment

erizocosmico commented Jun 5, 2018

kuba-- commented May 26, 2018 •

edited

Loading