Adds pg implementations for admin GetTree(id) and CreateTree #1305

vishalkuo · 2018-09-25T16:34:42Z

This PR introduces a partial implementation for admin storage in postgres. Particularly, it implements the CreateTree and GetTree(id) methods.

Much of the logic is borrowed heavily from the MySQL methods (including validation and setup) so I tried to pull out common helpers wherever I could. That being said, I think there were even further commonalities I could pull out but I worried that that'd be too big a change for this PR (let me know if you feel otherwise).

As far as testing, I copied most of the logic in mysql's storage tests but have yet to build our a test driver. I figured I'd leave that to another PR as this one was getting fairly large.

Issue: #1298

codecov · 2018-09-25T19:11:56Z

Codecov Report

Merging #1305 into master will decrease coverage by 0.77%.
The diff coverage is 7.07%.

@@            Coverage Diff             @@
##           master    #1305      +/-   ##
==========================================
- Coverage   61.09%   60.32%   -0.78%     
==========================================
  Files         125      127       +2     
  Lines       10070    10091      +21     
==========================================
- Hits         6152     6087      -65     
- Misses       3224     3324     +100     
+ Partials      694      680      -14

Impacted Files	Coverage Δ
storage/sql.go	`0% <0%> (ø)`
storage/mysql/admin_storage.go	`75.78% <100%> (+0.7%)`	⬆️
server/postgres_storage_provider.go	`4.34% <4.34%> (ø)`
testonly/hammer/hammer.go	`63.05% <0%> (-0.81%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f58fff5...cbc4d6a. Read the comment docs.

AlCutter

Hey Vishal, thanks for pushing on the PG implementation!
A few quick comments inline

AlCutter · 2018-09-26T13:45:51Z

server/postgres_storage_provider.go

+	"github.com/google/trillian/storage"
+	"github.com/google/trillian/storage/postgres"
+
+	// Load MySQL driver


Load PG driver :)

AlCutter · 2018-09-26T13:47:54Z

server/postgres_storage_provider.go

+)
+
+var (
+	pgConnStr         = flag.String("pg_uri", "user=postgres dbname=test port=5432 sslmode=disable", "Connection string for Postgres database")


Maybe change the flag name to match too?
It's not really a URI any more by the looks of things.

AlCutter · 2018-09-26T13:55:03Z

storage/postgres/admin_storage.go

+
+var (
+	// ErrNotImplemented is an error indicating a function's implementation has not been completed
+	ErrNotImplemented = errors.New("not implemented")


This can probably be unexported?

AlCutter · 2018-09-26T13:55:31Z

storage/postgres/testdb/testdb.go

+
+	"github.com/google/trillian/testonly"
+
+	_ "github.com/lib/pq" // mysql driver


postgres driver

AlCutter · 2018-09-26T13:56:41Z

storage/postgres/admin_storage_test.go

+	}
+}
+
+func cleanTestDB(db *sql.DB) {


Could pass T in here, and call t.Helper() as the first line, and t.Fatal rather than panic.

AlCutter · 2018-09-26T14:08:20Z

storage/postgres/tree_storage.go

+func OpenDB(dbURL string) (*sql.DB, error) {
+	db, err := sql.Open("postgres", dbURL)
+	if err != nil {
+		// Don't log uri as it could contain credentials


Don't log xxx

AlCutter · 2018-09-26T14:08:51Z

storage/storage_helpers.go

+import (
+	"database/sql"
+	"fmt"
+	time "time"


no need to rename time to time :)

AlCutter · 2018-09-26T14:09:48Z

storage/storage_helpers.go

+
+// FromMillisSinceEpoch converts an UNIX typestamp to a time struct
+func FromMillisSinceEpoch(ts int64) time.Time {
+	secs := int64(ts / 1000)


can just be return time.Unix(0, ts*1000000)

AlCutter · 2018-09-26T14:11:27Z

storage/storage_helpers.go

@@ -0,0 +1,162 @@
+// Copyright 2018 Google Inc. All Rights Reserved.


Maybe this should go under storage/sql or something - explicitly helpers for golang SQL compatible implementations?

AlCutter · 2018-09-26T14:13:43Z

storage/mysql/admin_storage.go

@@ -161,7 +158,7 @@ func (t *adminTX) GetTree(ctx context.Context, treeID int64) (*trillian.Tree, er
 	defer stmt.Close()

 	// GetTree is an entry point for most RPCs, let's provide somewhat nicer error messages.
-	tree, err := readTree(stmt.QueryRowContext(ctx, treeID))
+	tree, err := storage.ReadTree(stmt.QueryRowContext(ctx, treeID))


I don't know if this is a good idea or not...
OTOH it does promote reuse and reduce boilerplate, but on the other it forces some restrictions on implementations, maybe that's a good trade off to make until we discover a scenario where it doesn't work, let's see what others think.

@pphaneuf @Martin2112 @RJPercival @daviddrysdale

Yeah, definitely wasn't sure how much to pull out/leave in. My general understanding was that db/sql abstracted most differences away to only being at the query level but I could be wrong about that (especially with types, etc.)

RJPercival · 2018-09-26T14:54:35Z

storage/storage_helpers.go

+	return nil
+}
+
+// Row defines a common interface between sql.Row and sql.Rows(!), so we have to


Looks like this comment got a bit mangled. It was originally "There's no common interface between sql.Row and sql.Rows(!), so we have to define one."

RJPercival · 2018-09-26T14:55:53Z

storage/storage_helpers.go

+}
+
+// ValidateStorageSettings checks for storage settings and returns an error if they exist
+func ValidateStorageSettings(tree *trillian.Tree) error {


It doesn't seem like this should be a general helper, since storage_settings is supposed to be specific to individual storage implementations.

server/postgres_storage_provider.go

golangcibot · 2018-09-26T18:29:06Z

storage/postgres/testdb/testdb.go

+}
+
+// skipIfNoPG is a test helper that skips tests that require a local PG.
+func skipIfNoPG(t *testing.T) {


skipIfNoPG is unused

AlCutter · 2018-10-03T16:50:26Z

storage/sql.go

+	}
+
+	// Let's make sure we didn't mismatch any of the casts above
+	ok := tree.TreeState.String() == treeState


Maybe just make this:

ok := tree.TreeState.String() == treestate && tree.TreeType.String() == treeType && tree.HashStrategy.String() == hashStrategy && ...

?

AlCutter · 2018-10-04T11:38:17Z

Thanks, Vishal!

vishalkuo added 6 commits September 18, 2018 21:56

Adds sql creation scripts for postgres

555628d

Change sql types from bit varying to bytea

39e36d6

Change PK and add index order

84cbfc3

Merge branch 'master' of https://github.com/google/trillian

cbeaaba

Adds postgres admin create and get tree

d804ac3

Adds admin tests

dd32ad9

googlebot added the cla: yes label Sep 25, 2018

Fix lint errors

7e1c2f4

AlCutter reviewed Sep 26, 2018

View reviewed changes

RJPercival reviewed Sep 26, 2018

View reviewed changes

server/postgres_storage_provider.go Show resolved Hide resolved

RJPercival assigned vishalkuo Sep 26, 2018

Cleanup, fix incorrect comments

95fafac

golangcibot reviewed Sep 26, 2018

View reviewed changes

Remove unused function

05e5b80

vishalkuo force-pushed the vishal/pg-admin branch from 10b0858 to 670d565 Compare September 30, 2018 00:42

RJPercival requested review from RJPercival and AlCutter October 1, 2018 17:05

AlCutter reviewed Oct 3, 2018

View reviewed changes

vishalkuo force-pushed the vishal/pg-admin branch from 670d565 to 0941a7a Compare October 3, 2018 17:38

Trigger

cbc4d6a

vishalkuo force-pushed the vishal/pg-admin branch from 0941a7a to cbc4d6a Compare October 3, 2018 17:39

AlCutter approved these changes Oct 4, 2018

View reviewed changes

AlCutter merged commit bff2f3b into google:master Oct 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds pg implementations for admin GetTree(id) and CreateTree #1305

Adds pg implementations for admin GetTree(id) and CreateTree #1305

vishalkuo commented Sep 25, 2018 •

edited by RJPercival

Loading

codecov bot commented Sep 25, 2018 •

edited

Loading

AlCutter left a comment

AlCutter Sep 26, 2018

AlCutter Sep 26, 2018

AlCutter Sep 26, 2018

AlCutter Sep 26, 2018

AlCutter Sep 26, 2018

AlCutter Sep 26, 2018

AlCutter Sep 26, 2018

AlCutter Sep 26, 2018

AlCutter Sep 26, 2018

AlCutter Sep 26, 2018

vishalkuo Sep 26, 2018

RJPercival Sep 26, 2018

RJPercival Sep 26, 2018

golangcibot Sep 26, 2018

AlCutter Oct 3, 2018

AlCutter commented Oct 4, 2018


		"github.com/google/trillian/testonly"

		_ "github.com/lib/pq" // mysql driver

		@@ -0,0 +1,162 @@
		// Copyright 2018 Google Inc. All Rights Reserved.

Adds pg implementations for admin GetTree(id) and CreateTree #1305

Adds pg implementations for admin GetTree(id) and CreateTree #1305

Conversation

vishalkuo commented Sep 25, 2018 • edited by RJPercival Loading

codecov bot commented Sep 25, 2018 • edited Loading

Codecov Report

AlCutter 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

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

AlCutter commented Oct 4, 2018

vishalkuo commented Sep 25, 2018 •

edited by RJPercival

Loading

codecov bot commented Sep 25, 2018 •

edited

Loading