added/updated postgresql implementation of log_storage #1571

bigjimnolan · 2019-05-09T00:25:34Z

I have added the code to use the log_storage with postgres. This allows a third storage solution for the base set. I have not updated or implemented the map storage code yet, however.

Contributes to #1298.

Checklist

I have updated the CHANGELOG.
- Adjust the draft version number according to semantic versioning rules.
I have updated documentation accordingly.

…sting

codecov · 2019-05-09T04:48:10Z

Codecov Report

Merging #1571 into master will increase coverage by 0.82%.
The diff coverage is 61.78%.

@@            Coverage Diff             @@
##           master    #1571      +/-   ##
==========================================
+ Coverage   66.89%   67.71%   +0.82%     
==========================================
  Files         110      111       +1     
  Lines        8946     9529     +583     
==========================================
+ Hits         5984     6453     +469     
- Misses       2358     2383      +25     
- Partials      604      693      +89

Impacted Files	Coverage Δ
server/postgres_storage_provider.go	`4.16% <0%> (-0.19%)`	⬇️
storage/postgres/queue.go	`42% <42%> (ø)`
storage/postgres/tree_storage.go	`55.35% <51.61%> (+48.14%)`	⬆️
storage/postgres/log_storage.go	`64.4% <64.4%> (ø)`
log/sequencer.go	`74.89% <0%> (ø)`	⬆️
testonly/compact_merkle_tree.go
merkle/compact/tree.go	`89.83% <0%> (+0.64%)`	⬆️

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 c3b56b5...3ff3939. Read the comment docs.

RJPercival

Could you add test coverage please?

server/postgres_storage_provider.go

storage/postgres/tree_storage.go

storage/postgres/testdb/testdb.go

AlCutter

Thanks for sending this!

A few comments from a quick pass over.

storage/postgres/storage_unsafe.sql

storage/postgres/log_storage.go

storage/postgres/storage_unsafe.sql

storage/postgres/tree_storage.go

…storage

…abase via test code

bigjimnolan · 2019-05-10T13:36:05Z

The CI keeps failing on a goimports failure on a file I didn't edit. Can anyone offer advice on what's going on?

AlCutter · 2019-05-10T15:59:46Z

You can just run the goimports tool on that file and it should fix it for you

bigjimnolan · 2019-05-10T17:06:24Z

I absolutely agree that goimports should fix this issue. I don't think we're working with the same version. When I run goimports on an old version of the file, I get:

bigjim@CTDev:~/go/src/github.com/google/t2/server/admin$ goimports -d admin_server_test.go
diff -u admin_server_test.go.orig admin_server_test.go
--- admin_server_test.go.orig   2019-05-10 17:02:50.239530377 +0000
+++ admin_server_test.go        2019-05-10 17:02:50.239530377 +0000
@@ -596,8 +596,8 @@
        for _, test := range tests {
                setup := setupAdminServer(
                        ctrl,
-                       nil,                       /* keygen */
-                       false,                     /* snapshot */
+                       nil,   /* keygen */
+                       false, /* snapshot */
                        test.wantCode == codes.OK, /* shouldCommit */
                        false /* commitErr */)
                s := setup.server
@@ -810,8 +810,8 @@
        for _, test := range tests {
                setup := setupAdminServer(
                        ctrl,
-                       nil,                   /* keygen */
-                       false,                 /* snapshot */
+                       nil,   /* keygen */
+                       false, /* snapshot */
                        test.deleteErr == nil, /* shouldCommit */
                        test.commitErr /* commitErr */)
                req := &trillian.DeleteTreeRequest{TreeId: 10}
@@ -897,8 +897,8 @@
        for _, test := range tests {
                setup := setupAdminServer(
                        ctrl,
-                       nil,                     /* keygen */
-                       false,                   /* snapshot */
+                       nil,   /* keygen */
+                       false, /* snapshot */
                        test.undeleteErr == nil, /* shouldCommit */
                        test.commitErr /* commitErr */)
                req := &trillian.UndeleteTreeRequest{TreeId: 10}

It seems to loathe the tabs and wants spaces. However, when I run it on the file that autobuild is complaining about, I get no response:

bigjim@CTDev:~/new_trillian/trillian/server/admin$ goimports -d admin_server_test.go
bigjim@CTDev:~/new_trillian/trillian/server/admin$

Indicating, to me, at least that the code is fine. However, the build version of the formatter does not agree; even after removing the tabs. I have been able to get further by deleting the comments, but I don't think that's a nice thing to do with someone else's code. What is the issue with the comments?

AlCutter · 2019-05-14T17:31:29Z

Cool, can you put a Big Scary Warning™ (perhaps a comment and a log.Warning()) in here somewhere about it being experimental code and very likely to eat ones cat, etc. if used?

bigjimnolan · 2019-05-14T18:22:48Z

I have put it in the logs as a warning when people use the PostgreSQL log storage provider.

AlCutter

@RJPercival you cool?

AlCutter · 2019-05-16T16:02:41Z

@RJPercival you cool?

ping @RJPercival

RJPercival

Are the tests mostly copied and pasted from the MySQL tests? If so, it'd be better to refactor into a test suite that can be applied to either the MySQL or Postgres implementations. That'd be fine as a follow-up though.

There appear to be a number of comments from @AlCutter that haven't been resolved yet. Since you've approved already, does that mean you're happy for those to be deferred, @AlCutter?

RJPercival · 2019-05-17T12:41:48Z

server/admin/admin_server_test.go

+			nil,   // keygen
+			false, // snapshot
+			test.wantCode == codes.OK,
+			false)


Why did we lose the comments from some of these lines?

These comments were causing the server-side goimports to fail. I have no idea why. They checked out just fine on my local side. When I removed them, the autobuild succeeded. I have no problem with someone putting them back in.

storage/postgres/README.md

storage/postgres/storage.sql

storage/postgres/tree_storage.go

storage/postgres/storage_test.go

RJPercival · 2019-05-17T17:31:32Z

storage/postgres/storage_test.go

+		nodeIDsToRead[i] = nodesToStore[i].NodeID
+	}
+
+	{


Why the braces here?

Co-Authored-By: Rob Percival <[email protected]>

bigjimnolan · 2019-05-17T21:29:09Z

I have lost the ability to see what's different between my storage.sql and the repo's version. Can you please manually resolve those conflicts?

Thanks,

Jim

bigjimnolan

I've pushed a new version up, but I think we're still jammed up on storage.sql. I have no idea what its looking like on the repo side and I think you even moved it. Can you please make the fixes on your end, so I can re-sync and start pushing through my next series of updates?

Thanks,

Jim

RJPercival

LGTM bar a few unresolved comments and some future cleanup to share code with the MySQL implementation.

RJPercival · 2019-05-20T14:18:47Z

server/mock_log_operation.go

 	reflect "reflect"
+
+	gomock "github.com/golang/mock/gomock"


Did go generate move this import?

I guarantee it did. For some of the directories, I had to run goimports for all the files.

RJPercival · 2019-05-20T14:21:06Z

storage/postgres/storage_test.go

+}
+
+func openTestDBOrDie() *sql.DB {
+	db, err := testdb.NewTrillianDB(context.TODO())


If you look at the diff for TestMain(), you'll see that it used to create a Context and pass it to testdb.NewTrillianDBOrDie(). You could instead pass the context to openTestDBOrDie().

RJPercival · 2019-05-20T14:35:20Z

Oops, tried to push a merge as requested but got the command a little wrong and pushed my master branch onto yours, which happened to be where you were doing your work. Could you re-push your commits to your master branch please?

bigjimnolan · 2019-05-20T15:13:42Z

Can do

bigjimnolan · 2019-05-20T15:16:10Z

Done

bigjimnolan · 2019-05-21T15:51:43Z

Hey, I have re-pushed the code and got it through the docker tests ( I think the resource wasn't available last time it rand ) , so we should be good to go now. Thanks,
Jim

* master: (94 commits) Complete TODO (#1632) fake_node_reader: Remove unused field (#1631) Parallelize VerifyMapLeavesResponse (#1615) Remove redundant root hash calculations (#1630) sequencer: Consolidate compact.Tree initialization (#1629) Remove unused NodeReader method (#1625) Fix bazel build (#1627) added/updated postgresql implementation of log_storage (#1571) Clean up compact.Tree tests (#1622) Remove old hash list format from compact.Tree (#1621) Mention protoc-gen-doc in README.md merkle: Add testonly package with golden hashes (#1620) compact: Simplify getting hashes in NewTreeWithState (#1618) compact: Implement Tree using Range (#1522) Guard verbose logging in merkle path code (#1604) Make MaskLeft of NodeID return an explicit copy. (#1612) Convert directly from Int.Bits() to NodeID (#1614) Couple of changes to make NodeIDs more frugal. (#1613) compact.Tree: Change semantic of adding leaves (#1592) MapHasher: Do not return error from HashLeaf (#1611) ...

added/updated postgresql implementation of log_storage

7abee92

googlebot added the cla: yes label May 9, 2019

bigjimnolan added 5 commits May 9, 2019 03:10

reverted some test code to make commit compatible with integration te…

c9ac881

…sting

Typo fix in db code

af2cb5a

udpated queue batching code to work for postgres

d12bba1

updated code using gofmt and goimport

b79e24e

more format fixing

86a2de4

RJPercival reviewed May 9, 2019

View reviewed changes

RJPercival requested a review from AlCutter May 9, 2019 10:52

RJPercival assigned bigjimnolan May 9, 2019

AlCutter reviewed May 9, 2019

View reviewed changes

bigjimnolan added 11 commits May 9, 2019 21:51

Made updates referenced in pull request. Added test coverage for log …

93301be

…storage

Updated test code to explicitly declare funtion for AddLeaf call

733e86f

Updated test code and fixed methodology for pushing sql code into dat…

ed19758

…abase via test code

Updated log storage and added some more testing

55b8240

Trying to fix this odd error with compact.VisitFn

7509c43

ignoring test for now

628a6a3

used goimports

5e21ca2

updated for lint checks

b3d4fcd

fixing goimport random gripe

44b41c4

must be a different copy of goimports on the server

bde1d03

lint fixes?

b3767ad

bigjimnolan added 3 commits May 10, 2019 13:38

Seeing if this is the issue with goimports

a028c8b

will this pass the lint check?

d80b158

will this pass the lint check?

8c98f37

will this pass the lint check 3/?

a424960

RJPercival assigned RJPercival and AlCutter and unassigned bigjimnolan May 14, 2019

Added warning about experimental nature of PostgreSQL support

1004e83

AlCutter approved these changes May 14, 2019

View reviewed changes

RJPercival reviewed May 17, 2019

View reviewed changes

bigjimnolan and others added 7 commits May 17, 2019 11:57

Update storage/postgres/README.md

a750242

Co-Authored-By: Rob Percival <[email protected]>

Update storage/postgres/README.md

1acdaa0

Co-Authored-By: Rob Percival <[email protected]>

Update storage/postgres/storage.sql

59837d6

Co-Authored-By: Rob Percival <[email protected]>

Update storage/postgres/storage_test.go

16137cd

Co-Authored-By: Rob Percival <[email protected]>

Update storage/postgres/storage_test.go

e70d3ea

Co-Authored-By: Rob Percival <[email protected]>

Apply suggestions from code review

7bfb409

Co-Authored-By: Rob Percival <[email protected]>

Made updates based on pull request comments

e5807c5

bigjimnolan commented May 17, 2019

View reviewed changes

RJPercival approved these changes May 20, 2019

View reviewed changes

RJPercival closed this May 20, 2019

bigjimnolan added 2 commits May 20, 2019 15:14

updated readme.txt

714e7fb

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

96f9796

RJPercival reopened this May 20, 2019

udpated readme to restart build

3ff3939

RJPercival merged commit a3f4ab2 into google:master May 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added/updated postgresql implementation of log_storage #1571

added/updated postgresql implementation of log_storage #1571

bigjimnolan commented May 9, 2019 •

edited by RJPercival

Loading

codecov bot commented May 9, 2019 •

edited

Loading

RJPercival left a comment

AlCutter left a comment

bigjimnolan commented May 10, 2019

AlCutter commented May 10, 2019

bigjimnolan commented May 10, 2019 •

edited by RJPercival

Loading

AlCutter commented May 14, 2019

bigjimnolan commented May 14, 2019

AlCutter left a comment

AlCutter commented May 16, 2019

RJPercival left a comment

RJPercival May 17, 2019

bigjimnolan May 17, 2019

RJPercival May 17, 2019

bigjimnolan commented May 17, 2019

bigjimnolan left a comment

RJPercival left a comment

RJPercival May 20, 2019

bigjimnolan May 20, 2019

RJPercival May 20, 2019

RJPercival commented May 20, 2019

bigjimnolan commented May 20, 2019

bigjimnolan commented May 20, 2019

bigjimnolan commented May 21, 2019

added/updated postgresql implementation of log_storage #1571

added/updated postgresql implementation of log_storage #1571

Conversation

bigjimnolan commented May 9, 2019 • edited by RJPercival Loading

Checklist

codecov bot commented May 9, 2019 • edited Loading

Codecov Report

RJPercival left a comment

Choose a reason for hiding this comment

AlCutter left a comment

Choose a reason for hiding this comment

bigjimnolan commented May 10, 2019

AlCutter commented May 10, 2019

bigjimnolan commented May 10, 2019 • edited by RJPercival Loading

AlCutter commented May 14, 2019

bigjimnolan commented May 14, 2019

AlCutter left a comment

Choose a reason for hiding this comment

AlCutter commented May 16, 2019

RJPercival left a comment

Choose a reason for hiding this comment

RJPercival May 17, 2019

Choose a reason for hiding this comment

bigjimnolan May 17, 2019

Choose a reason for hiding this comment

RJPercival May 17, 2019

Choose a reason for hiding this comment

bigjimnolan commented May 17, 2019

bigjimnolan left a comment

Choose a reason for hiding this comment

RJPercival left a comment

Choose a reason for hiding this comment

RJPercival May 20, 2019

Choose a reason for hiding this comment

bigjimnolan May 20, 2019

Choose a reason for hiding this comment

RJPercival May 20, 2019

Choose a reason for hiding this comment

RJPercival commented May 20, 2019

bigjimnolan commented May 20, 2019

bigjimnolan commented May 20, 2019

bigjimnolan commented May 21, 2019

bigjimnolan commented May 9, 2019 •

edited by RJPercival

Loading

codecov bot commented May 9, 2019 •

edited

Loading

bigjimnolan commented May 10, 2019 •

edited by RJPercival

Loading