Support pubsub methodology in SSE requests by gadisr · Pull Request #5 · kinecosystem/go

gadisr · 2018-08-28T11:02:14Z

Every connection subscribes only to related data and get notified on changes accordingly.
This optimizes Horizon<->History DB throughput and make flow more effiecient.

oryband · 2018-08-28T11:39:39Z

use log fields for the "topic" field like in logrus. check usage throughout the codebase

oryband · 2018-08-28T11:40:27Z

redundant line is redundant. use git add -p

oryband · 2018-08-28T11:40:55Z

channel is a generic name. call this ssePumped or something more verbose

oryband · 2018-08-28T11:41:16Z

comment is confusing:

what do you mean by "get an if from requests"?

what is "q topic"?

oryband · 2018-08-28T11:42:34Z

does it make sense to use a switch {} block instead? or do you need to evaluate all conditions all every on call to this function?

oryband · 2018-08-28T12:26:29Z

why do you have to create a new goroutine here? doesn't it just overcomplicate things?

Changed implementation

oryband · 2018-08-28T12:27:12Z

"Horizon" database instead of "History"

oryband · 2018-08-28T12:29:43Z

will using this function work properly if there are multiple subscribers to the same topic?

Fixed to use different pubsub implementation

oryband · 2018-08-28T12:40:37Z

just make two select blocks instead of an infinitie for loop. the first blook will continue successfully if channel_a returns, and then just run the select again and succeed if 2 seconds pass.

oryband · 2018-08-28T12:41:05Z

are you going to test specific functions along with testing the entire flow?

oryband · 2018-09-02T14:06:07Z

is it ok for the switch to not catch any catch any condition?

oryband · 2018-09-02T14:06:55Z

channel name doesn't convey any information beside its type, and makes it hard to follow your flow down the road. better give it a meaningful name like topicMessage or something

oryband · 2018-09-02T14:09:44Z

rephrasing and small typo fixes: putting result before the condition for better readability:

Subscribe this handler to the topic if the SSE request is related to a specific topic (tx_id, account_id, etc.). This causes the request to only be triggered by this topic. Unsubscribe when done.

Also, to which request are you referring on "request will be triggered" - HTTP or database query?

oryband · 2018-09-02T14:10:54Z

why int32 --> int --> string instead of just straight to string?

oryband · 2018-09-02T14:12:05Z

package name is already pubsub, no need to rename to pubsub again

oryband · 2018-09-02T14:59:20Z

where did this magic value come from?

oryband · 2018-09-02T14:59:58Z

same comments as previous test

oryband · 2018-09-02T15:00:32Z

i don't understand this func doc. if it's from another file why not just import it?

oryband · 2018-09-02T15:02:38Z

where is the New function coming from? from the . import? if so, can you not rely on the . import? confusing

oryband · 2018-09-02T15:03:53Z

similar to comment in the test above, duplicate this test to another one that publishes more than one value

oryband · 2018-09-09T08:40:25Z

need to check for if !ok first, otherwise this would crash the program. if !ok then action == null. Replace this condition with the one coming just after it.

oryband · 2018-09-09T08:51:10Z

why are you doing this stahp this unnecessary diff noise

also another one like this in the import section at the top of this file

oryband · 2018-09-09T09:04:42Z

why not action.GetString(..) like all other implementations? there are other functions like this in this pr

There is no value for ledger there, just "ledger" hardcoded itself

There is no value in this action for specific ledger_id, so registration topic is a general change in the ledger.

oryband · 2018-09-09T09:05:32Z

are you sure the arg here should be account_id? it's already used in func (action *DataShowAction) GetTopic()

oryband · 2018-09-09T09:05:57Z

avoid calling GetString() twice:

func (action *EffectIndexAction) GetTopic() (res string) { if res := action.GetString(...); res != "" { return } if res := ...; res != "" { return } // repeat for other fields return // res == ""

apply this to other functions in this pr

oryband · 2018-09-09T09:34:51Z

spaghetti code: instead of doing a select inside a select, just have two select blocks one after the other. i already wrote this comment here somewhere look it up

oryband · 2018-09-09T09:39:39Z

rename channels to topics

oryband · 2018-09-09T09:40:38Z

many subscribers

oryband · 2018-09-09T09:40:54Z

what are these magic address and kahuna?

oryband · 2018-09-09T09:41:48Z

bug: if the time.After returns before subscription the wait at the bottom will hang indefinitely

oryband · 2018-09-12T09:00:51Z

also, magic value "commit"

oryband · 2018-09-12T09:02:06Z

magic number

Every connection subscribes only to related data and get notified on changes accordingly. This optimizes Horizon<->History DB throughput and make flow more effiecient.

… after data is written on DB. Also changed sse channel from struct{} to interface{} to be easily used with pubsub solution.

Changed tests by pr and added undubscribe when each test is done

…stead

all these changes will be included in this branch since they all belong to the same related long term solution for sse optimization

In order to optimize horizon<->DB throughput, avoid running sse procedures if no change happen in ledger state.

…ecify number of db connections to use.

…sse events

NOTE This only happens only healthy HTTP 2xx responses. This is a hacky solution that tries to solve the case where clients have an unwanted behavior: They already have an account open and just want to poll their account balance but retry too quickly, according to default retry value (a few seconds usually). This causes them to spam the server right after disconnecting the SSE stream. This change will cause them to receive the preamble over and over again on every SSE tick, practically turning it into a keepalive message. This would lower the reconnection amount and in turn (most importantly) the amount of queries sent to the Horizon database. This has a cost though - it keeps SSE connections alive and also causes "junk" keep alive traffic to be sent over and over again.

This reverts commit 34ea97e.

oryband · 2018-10-16T12:35:49Z

cleaning this up and closing in favor of #14

gadisr requested review from doodyparizada and oryband August 28, 2018 11:02

oryband suggested changes Aug 28, 2018

View reviewed changes

gadisr force-pushed the gadi/add_pubsub_to_sse branch from 6e9c29b to 613a098 Compare August 30, 2018 13:41

oryband suggested changes Sep 2, 2018

View reviewed changes

gadisr force-pushed the gadi/add_pubsub_to_sse branch from a4a9da5 to 0b51059 Compare September 9, 2018 07:11

oryband suggested changes Sep 9, 2018

View reviewed changes

gadisr force-pushed the gadi/add_pubsub_to_sse branch from 968878d to 51dafea Compare September 9, 2018 11:33

oryband suggested changes Sep 12, 2018

View reviewed changes

oryband approved these changes Sep 12, 2018

View reviewed changes

Gadi Srebnik and others added 9 commits September 17, 2018 17:47

Support pubsub methodology in SSE requests

945bea9

Every connection subscribes only to related data and get notified on changes accordingly. This optimizes Horizon<->History DB throughput and make flow more effiecient.

Move pubsub to use https://github.com/cskr/pubsub and move trigger to…

e19981e

… after data is written on DB. Also changed sse channel from struct{} to interface{} to be easily used with pubsub solution.

Changes by pr

29ac063

Add more tests

505650f

Moved GetTopic to SSE interface to be handled by each action

bbd6c33

Changed tests by pr and added undubscribe when each test is done

Fixes for PR

50f6306

add github.com/cskr/pubsub to glide.yaml; update glide.lock

0d092c4

cosmetic updates - docs and readability

52e2046

don't use magic values and numbers, const them at the top of files in…

5280130

…stead

oryband force-pushed the gadi/add_pubsub_to_sse branch from c8c631c to 5280130 Compare September 17, 2018 14:47

oryband and others added 10 commits September 17, 2018 18:46

fix wrong log import in horizon/internal/render/sse/main.go

9a2211a

Revert multiple branches #10-#7, #4, #2

be5fcbb

all these changes will be included in this branch since they all belong to the same related long term solution for sse optimization

Restrict sse tick for ledger changes only

415b24c

In order to optimize horizon<->DB throughput, avoid running sse procedures if no change happen in ledger state.

cosmetic update to previous commit

96ebea2

Added the '[horizon|core]-db-[open|idle]-max-connections' flags to sp…

2ef7150

…ecify number of db connections to use.

Make publish non-blocking write to the subscribed channel

5d923cd

update documentation from last commit

6741117

log print update in sse code

0255acc

typo fixes to comments

2d5b3e4

log channel id (its address) when logging topic subscribe / wait for …

9b716ff

…sse events

oryband added 3 commits October 3, 2018 13:09

rm redundant ingestion log line

5cd3ead

Revert "use SSE reply preamble as keep alive message"

1641d29

This reverts commit 34ea97e.

oryband mentioned this pull request Oct 16, 2018

SSE requests us Pub/Sub methodology, more efficient #14

Merged

oryband closed this Oct 16, 2018

oryband deleted the gadi/add_pubsub_to_sse branch April 4, 2019 12:44

Conversation

gadisr commented Aug 28, 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!

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!

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!

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!

oryband Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

oryband Sep 9, 2018 •

edited

Loading