Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/implement autopaho rpc #98

Merged
merged 13 commits into from
Aug 26, 2022
Merged

Feature/implement autopaho rpc #98

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2db2542
Fix router unit test for shared subscriptions
bkneis Aug 9, 2022
e4fda94
Add build commands to makefile and ignore bin
bkneis Aug 9, 2022
e818d6a
Fix paho RPC extension using context.Background
bkneis Aug 9, 2022
6eedf91
Implement RPC extension for autopaho
bkneis Aug 9, 2022
8780381
Add function to use the underlying client in connection manager in a …
bkneis Aug 9, 2022
e66bbf6
Remove .env from rpc example
bkneis Aug 15, 2022
1b134ad
Remove UseClient
bkneis Aug 15, 2022
10f9f48
Update RPC example with passing router through client
bkneis Aug 15, 2022
e095ad9
Implement PR feedback
bkneis Aug 16, 2022
5549e39
Update reference to response topic in autopaho rpc extension
bkneis Aug 16, 2022
3d51531
Rename to ResponseTopicFmt
bkneis Aug 16, 2022
ace041a
Remove redudant need to pass router
bkneis Aug 19, 2022
c650e72
Prevent blocking on recieving a response by using a select with context
bkneis Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions autopaho/auto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,11 @@ func NewConnection(ctx context.Context, cfg ClientConfig) (*ConnectionManager, e
if cli == nil {
break mainLoop // Only occurs when context is cancelled
}
r := c.cli.Router // Get a reference to the previous client's Router
c.mu.Lock()
c.cli = cli
if r != nil {
if cfg.Router != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you are implementing the change I suggested but Sorry - I meant before the cliCfg := cfg. That way it will get used for every connection via:

establishBrokerConnection ->
       paho.NewClient ->
            c := &Client{ ... ClientConfig: conf, ...

(there would then be no need to set c.cli.Router directly). It would make sense to do this at the top of the function (as we check/edit other parts of the config struct there = e.g. something like

if cfg.ConnectTimeout == 0 {
   cfg.ConnectTimeout = 10 * time.Second
}
if cfg.Router == nil { // Important that the router survives reconnections
   cfg.Router = paho.NewStandardRouter()
}

Sorry - it's late here so just had a really quick look so apologies if I misunderstood your intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MattBrittan I've just updated the PR, I did not realise that the cfg being passed to the NewClient would then persist the router, so there is no need to assign it explicitly as you mentioned. Please let let me know of any other changes you would like :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually something that should be changed (not necessarily as part of your PR). If the user passes a nil Router then the router will be recreated every-time we reconnect which is likely to be unexpected. I have no issues with this change (its basically just introducing a new example now). @alsm are you happy with the changes to the paho rpc demo and router_test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MattBrittan yeah I'm happy with the rpc changes. Sorry I didn't respond quicker, I'm away on vacation in Norway atm and will have intermittent connectivity for another week.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alsm - I have gone ahead and merged. Suggested a couple of minor changes but they are not essential and can be handled in another PR (this one has been open long enough). I hope you have a great break!

// Assign the previous client's router to the new c.cli to persist the registered handlers through reconnects
c.cli.Router = r
c.cli.Router = cfg.Router
}
c.mu.Unlock()
close(c.connUp)
Expand Down