Pilosa index driver as library #308

kuba-- · 2018-07-31T14:13:36Z

New implementation of index driver (pilosalib). This one also uses pilosa but as a embedded library (comparing to external http server used by pilosa driver)

It closes
#302

kuba-- · 2018-07-31T14:34:04Z

Build fails because of:

../../../github.com/pilosa/pilosa/cluster.go:1544:18: multiple-value uuid.NewV4() in single-value context
../../../github.com/pilosa/pilosa/holder.go:529:22: multiple-value uuid.NewV4() in single-value context

We have to start vendoring dependencies also for libraries!

erizocosmico · 2018-07-31T15:24:46Z

We have to start vendoring dependencies also for libraries!

Yup. But in this case, we should just upgrade to whatever's the latest version of uuid.

Signed-off-by: kuba-- <[email protected]>

kuba-- · 2018-07-31T22:37:29Z

Build fixed.

kuba-- · 2018-07-31T22:39:01Z

server/context.go

-		cancel()
-		return nil, nil, err
-	}
+	id := uuid.NewV4()


it complaint because the newest version doesn't return an error

erizocosmico · 2018-08-01T07:02:54Z

.travis.yml

-  - go get -u github.com/pilosa/go-pilosa
-  - cd "$GOPATH/src/github.com/pilosa/go-pilosa" && git checkout v0.9.0 && cd "$TRAVIS_BUILD_DIR"
+  - go get -u github.com/golang/dep/cmd/dep
+  - cd "$TRAVIS_BUILD_DIR"


i think we no longer need this

you mean cd "$TRAVIS_BUILD_DIR"? So far I left it just in case.

yep. it was because it used to CD into go-pilosa's dir. Now we don't need that anymore

erizocosmico · 2018-08-01T07:03:04Z

.travis.yml

-  - cd "$GOPATH/src/github.com/pilosa/go-pilosa" && git checkout v0.9.0 && cd "$TRAVIS_BUILD_DIR"
+  - go get -u github.com/golang/dep/cmd/dep
+  - cd "$TRAVIS_BUILD_DIR"
+  - touch Gopkg.toml


we could just dep init

I tried dep init but the problem is that it creates Gopkg files and locks the revision. After that any dep ensure -add or dep ensure -update doesn't work when I try to upgrade to the master (because lock is already set).
And for pilosa/pilosa we need to use master (waiting for 1.0.2).

erizocosmico · 2018-08-01T07:03:35Z

.travis.yml

+  - go get -u github.com/golang/dep/cmd/dep
+  - cd "$TRAVIS_BUILD_DIR"
+  - touch Gopkg.toml
+  - dep ensure -v -add "github.com/pilosa/[email protected]" "github.com/pilosa/pilosa@master"


maybe we could set the specific revision of pilosa instead, so we're in control if something changes

Yup, they promised to release 1.0.2 y'day, but nothing happened, so I pulled master.
The plan is maybe to use dep init and set up certain version (1.0.2+, because current 1.0.1 doesn't work for us).

We can still pin a revision, even if it's not a release, right? I mean pinning it to the hash of current master

Signed-off-by: kuba-- <[email protected]>

kuba-- · 2018-08-01T08:09:16Z

Removed TRAVIS_BUILD_DIR. Either we plan to review and merge (what works with pilosa@master) or wait for [email protected] which in theory should be released on Tuesday (y'day) ;)

Signed-off-by: kuba-- <[email protected]>

ajnavarro · 2018-08-01T08:29:39Z

sql/index/pilosa/driver.go

 		})
 		log.Warn("could not read index file, index is corrupt and will be deleted")

-		if err := os.RemoveAll(path); err != nil {
-			log.Warn("unable to remove folder of corrupted index")
+		if err := os.RemoveAll(configfile); err != nil {


I would prefer to delete the path instead of each file separately. Why we change it?

The problem here is folder's structure. We don't have id subfolder anymore. Just all db/table indexes go to the one pilosa index folder.
I wanted to keep the id folder for resources like config, mapping, etc. but it will distract pilosa/pilosa, because when you load an index in pilosa it goes through subfolders (and don't validate it), so at the end it creates fake/invalid extra indexes. That's why I flatten the structure and left subfolders to pilosa internals.

But right now I thought about another folders refactoring.
How about:

root |-db |-table |-[driver id] # pilosa folder if needed |-[id 1] # index metadata (config, mapping, ...) |-[id 2] ...

The pros. here is the structure is almost what we had before, so deleting the index is easier - you can just delete a id folder.
The cons. yo don't have connection between id and driver. If you have multiple indexes on the same table from different drivers you don't know which index belongs to which driver. What in theory may not be problem if we mark it in a config file.
WDYT?

ajnavarro · 2018-08-01T08:30:08Z

sql/index/pilosa/driver.go

 }

 // Delete the index with the given path.
 func (d *Driver) Delete(idx sql.Index) error {
-	path := filepath.Join(d.root, idx.Database(), idx.Table(), idx.ID())
-	if err := os.RemoveAll(path); err != nil {
+	if err := os.RemoveAll(d.configFileName(idx.Database(), idx.Table(), idx.ID())); err != nil {


The same as the previous comment.

ajnavarro · 2018-08-01T08:31:59Z

sql/index/pilosa/driver_test.go

@@ -44,7 +44,7 @@ func TestID(t *testing.T) {
 func TestLoadAll(t *testing.T) {
 	require := require.New(t)

-	path, err := ioutil.TempDir(os.TempDir(), "indexes")
+	path, err := ioutil.TempDir(os.TempDir(), "indexes-")


should we set a path called go-mysql-server on temp dir to be able to delete it at the end of every test?

The name is irrelevant to delete them at the end of every test. You just have to os.RemoveAll(PathReturnedByTempDir)

yeah, we should at least make it consistent across all tests.

Added setup, cleanup functions for tests' tmp dirs.

ajnavarro · 2018-08-01T08:32:14Z

sql/index/pilosa/driver_test.go

@@ -94,7 +94,7 @@ func TestSaveAndLoad(t *testing.T) {

 	db, table, id := "db_name", "table_name", "index_id"
 	expressions := makeExpressions("lang", "hash")
-	path, err := ioutil.TempDir(os.TempDir(), "indexes")
+	path, err := ioutil.TempDir("", "indexes")


the same as the previous comment.

kuba-- · 2018-08-01T08:33:17Z

I've locked pilosa to the commit hash: edf6fa9c67caeab6019fb17ab82425a74fdfaf87

Signed-off-by: kuba-- <[email protected]>

kuba-- · 2018-08-01T13:57:59Z

I have a feeling that following folder structure will be better for us (and more backward compatible) - WDYT?

root/
└── db
    └── table
        ├── .pilosa #pilosa special folder
        │   └── idx-d9b85cddd6ac716f0326c32c7ba4bd9ae2aeb558
        │       └── fld-daef88b79b2d1e52c779c70d4aa814546a1b10c2
        │           └── views
        │               ├── fragments
        │               │   ├── 0
        │               │   └── 0.cache
        │               └── standard
        ├── idx_1 #index id_1 metadata folder
        │   ├── .processing
        │   ├── config.yml
        │   └── mapping.db
        └── idx_2 #index id_2 metadata folder
            ├── config.yml
            └── mapping.db

ajnavarro · 2018-08-01T14:12:24Z

@kuba-- can you rebase please? (I merged first the ExpressionHash change than this one, sorry... :S)

ajnavarro · 2018-08-01T14:13:08Z

@kuba-- LGTM the proposed folder structure.

ajnavarro · 2018-08-01T14:13:59Z

Also, can you check the documentation to check some inconsistences? thanks!

kuba-- · 2018-08-01T14:23:06Z

I'm in the middle of resolving conflicts. After that I'll refactor folders structure and check docs.
A good thing about a new structure is backward compatibility, so I hope nothing has to be changed in docs (apart from adding a new driver which won't require pilosa server)

Signed-off-by: kuba-- <[email protected]>

kuba-- · 2018-08-02T09:14:03Z

@ajnavarro - ready to merge.

kuba-- added enhancement New feature or request proposal proposal for new additions or changes labels Jul 31, 2018

kuba-- requested a review from a team July 31, 2018 14:13

kuba-- mentioned this pull request Jul 31, 2018

Implement a new pilosa (server-less) index driver #302

Closed

ajnavarro mentioned this pull request Jul 31, 2018

Pilosa index driver as a library #307

Closed

Pilosa index driver as library

a94937c

Signed-off-by: kuba-- <[email protected]>

kuba-- commented Jul 31, 2018

View reviewed changes

erizocosmico suggested changes Aug 1, 2018

View reviewed changes

mcarmonaa approved these changes Aug 1, 2018

View reviewed changes

remove TRAVIS_BUILD_DIR

6f05933

Signed-off-by: kuba-- <[email protected]>

Lock pilosa to edf6fa9c67

53a1de2

Signed-off-by: kuba-- <[email protected]>

ajnavarro reviewed Aug 1, 2018

View reviewed changes

Cleanup test tmp dir.

4c245e5

Signed-off-by: kuba-- <[email protected]>

erizocosmico approved these changes Aug 1, 2018

View reviewed changes

kuba-- added the wip work in progress label Aug 1, 2018

kuba-- added 2 commits August 1, 2018 21:17

Get rid of ExpressionsHash

16259a7

Signed-off-by: kuba-- <[email protected]>

Refactor index folder structure.

198abbe

Signed-off-by: kuba-- <[email protected]>

kuba-- removed the wip work in progress label Aug 2, 2018

ajnavarro approved these changes Aug 2, 2018

View reviewed changes

ajnavarro merged commit f813628 into src-d:master Aug 2, 2018

kuba-- deleted the pilosa-lib branch August 2, 2018 09:15

kuba-- mentioned this pull request Aug 23, 2018

Gitbase index persistence and pilosa src-d/engine-deprecated#13

Closed

Pilosa index driver as library #308

Pilosa index driver as library #308

Uh oh!

Conversation

kuba-- commented Jul 31, 2018

Uh oh!

kuba-- commented Jul 31, 2018

Uh oh!

erizocosmico commented Jul 31, 2018

Uh oh!

kuba-- commented Jul 31, 2018

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!

kuba-- commented Aug 1, 2018

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!

kuba-- commented Aug 1, 2018

Uh oh!

kuba-- commented Aug 1, 2018

Uh oh!

ajnavarro commented Aug 1, 2018

Uh oh!

ajnavarro commented Aug 1, 2018

Uh oh!

ajnavarro commented Aug 1, 2018

Uh oh!

kuba-- commented Aug 1, 2018

Uh oh!

kuba-- commented Aug 2, 2018

Uh oh!

Uh oh!