Introduce cache and more pagination. #180

jholdstock · 2020-04-08T15:31:18Z

As discussed on Matrix, bundling changes from #173 and #174 into a new PR with some additional changes.

New style errors

This PR implements the new error modals as defined by @MariaPleshkova in https://xd.adobe.com/view/a814493c-193a-4dfc-5735-0e6d459a6924-de25/. These errors are displayed when an invalid URL is visited, for example /account?address=NotARealAddress. When you are searching for an account using the AJAX form, the errors should be displayed as before, this PR does not change those.

Note: This PR does not change the Javascript warning - that will need some extra thought because it cannot be implemented as a modal - drawing a modal depends on Javascript.

Cache

Cache has been introduced, which acts as a data buffer between hub and gui. Cache has setter functions which are used by the hub, and getter functions which are used by gui. It holds pre-formatted data which is ready to be served directly to gui without needing to lock any mutexes in the hub or hit the database. This facilitates implementing #167 because now the hub is able to push data into the cache when it knows the data has changed, whereas the previous model had the gui pulling data from the hub on a timer, with no knowledge of whether the underlying data had changed or not.

Pagination

Pagination has been added for both the Mined Blocks and the Connected Clients tables on the account page.

Rate Limiting

Rate limiting moved to a middleware which removes a lot of duplicated code.

As a result, rate limiting has been added to some places it was previously missing:

/ws the websocket route
/logout for admin logout

The limit has been increased from 3 to 5 for GUI clients. This is because some of the pages are now making extra requests (pagination, establishing a websocket, AJAX form submissions) and the limit of 3 was causing issues.

gui/assets/public/css/pagination.css

dnldd · 2020-04-13T12:32:21Z

gui/gui.go

+
+	guiRouter.HandleFunc("/", ui.Homepage).Methods("GET")
+	guiRouter.HandleFunc("/account", ui.Account).Methods("GET")
+	guiRouter.HandleFunc("/account_exist", ui.IsPoolAccount).Methods("GET")


Perhaps this route could be /account/exist, /poolaccount, /isaccount or /exist? These options conform to the route definitions structure already in place, account_exist, with that underscore does not.

Good catch. Reading further on this topic I believe a more proper solution would be to use the same /account URL as the page itself, but change the request type to HEAD.

https://stackoverflow.com/questions/21087913/proper-route-for-checking-resource-existence-in-a-restful-api
also links through to https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html (section 9.4 is useful)

dnldd · 2020-04-13T12:33:31Z

gui/gui.go

-		ui.poolHash = hashString(poolHash)
-		ui.poolHashMtx.Unlock()
-		return nil
+	// Initalise the cache


Missing comment punctuation.

dnldd · 2020-04-13T12:44:02Z

pool/hub.go

 // FetchClientInfo returns connection details about all pool clients.
-func (h *Hub) FetchClientInfo() map[string][]*ClientInfo {
-	clientInfo := make(map[string][]*ClientInfo)
+func (h *Hub) FetchClientInfo() []*Client {


If you're going to maintain the modifications to this function then the function signature will have to change some more. Since it's returning a slice of clients now I don't think it should be called FetchClientInfo anymore, perhaps FetchHubClients or FetchClients would do. Also the name allClients seems unnecessary here, you could call the client slice clients and enumerate the endpoint with for _, c := range endpoint.clients, this way you can append to the slice like this: clients = append(clients, c). I think this is readable without being verbose.

dnldd · 2020-04-13T12:47:34Z

pool/acceptedwork_test.go

@@ -93,27 +93,14 @@ func testAcceptedWork(t *testing.T, db *bolt.DB) {
 			fetchedWork.Height, workC.Height)
 	}

-	// Ensure requesting negative or zero most recent accepted work returns
-	// an error.
-	_, err = listMinedWorkByAccount(db, string(id), -1)


Can we keep listMinedWorkByAccount around just for these tests?

listMinedWorkByAccount was called three times in test code.

Two of these appearances were just testing the behaviour of listMinedWorkByAccount itself, so I wont bother to reinstate those.

The third call was to ensure mined work is persisted in the database correctly, so this one seems worth reinstating. I will put back some code which performs this check.

dnldd

Not done with my review, I have a couple more diffs to get through. Will update when I'm done. Regarding the cache update I think It's on the right path, we can pass a channel to the gui to read for cache update signals and process them accordingly.

dnldd · 2020-04-13T22:06:52Z

gui/gui.go

-	// FetchClientInfo returns connection details about all pool clients.
-	FetchClientInfo func() map[string][]*pool.ClientInfo
+	// FetchClientInfo returns details about all connected pool clients.
+	FetchClientInfo func() []*pool.Client


This would have to be updated as well as the description if you make the changes suggested to the function signature.

dnldd · 2020-04-13T22:18:56Z

gui/cache.go

+	poolHashMtx sync.RWMutex
+
+	clients    map[string][]client
+	clientsMtx sync.RWMutex


Don't think it's necessary to group struct fields like this, I think the codebase only has atomically updated fields separated from the rest.

You mean the new lines? I included those just for the sake of readability and making it easier to see that each field has its own mutex, but I am happy to remove them.

dnldd · 2020-04-13T22:20:22Z

Done reviewing.

jholdstock · 2020-04-16T09:54:05Z

Thanks @dnldd, I've pushed commits implementing review changes.

dnldd · 2020-04-16T10:13:20Z

pool/hub.go

-// FetchClientInfo returns connection details about all pool clients.
-func (h *Hub) FetchClientInfo() []*Client {
-	allClients := make([]*Client, 0)
+// FetchClients returns connection details about all pool clients.


// FetchClients returns all connected pool clients.

dnldd · 2020-04-16T10:13:36Z

gui/gui.go

@@ -76,8 +76,8 @@ type Config struct {
 	FetchWorkQuotas func() ([]*pool.Quota, error)
 	// BackupDB streams a backup of the database over an http response.
 	BackupDB func(w http.ResponseWriter) error
-	// FetchClientInfo returns details about all connected pool clients.
-	FetchClientInfo func() []*pool.Client
+	// FetchClients returns details about all connected pool clients.


// FetchClients returns all connected pool clients.

dnldd · 2020-04-16T10:25:07Z

Minor comment updates and we're good to go @jholdstock

jholdstock · 2020-04-16T10:33:21Z

Done

jholdstock added 7 commits March 30, 2020 13:41

gui: Implement new style errors.

9b7d06e

gui: Account page to use work cache rather than db

09a2dad

gui: Pagination for blocks mined by account

9b0ff5f

gui: Move cache into its own file

93c5c14

gui: Move rate limiting to middleware

ba8d870

gui: Pagination for account clients

aa829b5

gui: Remove unnecessary ClientInfo from hub

21666fc

This was referenced Apr 8, 2020

Pagination for blocks mined by account #174

Closed

gui: Implement new style errors. #173

Closed

gui: Increase rate limits for gui clients

064980e

jholdstock force-pushed the pagination6 branch from e807226 to 064980e Compare April 8, 2020 15:44

dnldd reviewed Apr 13, 2020

View reviewed changes

gui/assets/public/css/pagination.css Show resolved Hide resolved

dnldd reviewed Apr 13, 2020

View reviewed changes

dnldd requested changes Apr 13, 2020

View reviewed changes

dnldd reviewed Apr 13, 2020

View reviewed changes

jholdstock added 4 commits April 16, 2020 10:51

gui: Check account existence with HEAD request

983c6fa

gui: Rename FetchClientInfo

a46040a

pool: Test mined work is persisted to database.

53fae80

gui: Remove newlines between cache fields

6800dd0

dnldd reviewed Apr 16, 2020

View reviewed changes

gui: Update comments

8594180

dnldd approved these changes Apr 16, 2020

View reviewed changes

jholdstock merged commit 167fa18 into decred:master Apr 16, 2020

jholdstock deleted the pagination6 branch October 6, 2020 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce cache and more pagination. #180

Introduce cache and more pagination. #180

jholdstock commented Apr 8, 2020

dnldd Apr 13, 2020

jholdstock Apr 16, 2020

dnldd Apr 13, 2020

dnldd Apr 13, 2020

dnldd Apr 13, 2020

jholdstock Apr 16, 2020

dnldd left a comment

dnldd Apr 13, 2020

dnldd Apr 13, 2020 •

edited

Loading

jholdstock Apr 16, 2020

dnldd commented Apr 13, 2020

jholdstock commented Apr 16, 2020

dnldd Apr 16, 2020

dnldd Apr 16, 2020

dnldd commented Apr 16, 2020

jholdstock commented Apr 16, 2020

Introduce cache and more pagination. #180

Introduce cache and more pagination. #180

Conversation

jholdstock commented Apr 8, 2020

New style errors

Cache

Pagination

Rate Limiting

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

dnldd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnldd Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnldd commented Apr 13, 2020

jholdstock commented Apr 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnldd commented Apr 16, 2020

jholdstock commented Apr 16, 2020

dnldd Apr 13, 2020 •

edited

Loading