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.

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

v23-beta #476

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
5 participants
@adrianhopebailie
Copy link
Contributor

commented Dec 7, 2018

Refactor connector for more flexible config and ability to cluster multiple instances for a composite connector.

This is the outcome of the experiments done by @DonChangfoot @matdehaast and me over the last few weeks.

It is not quite ready for release but we'd like to get feedback asap.

Still TODO is to add integration tests for new topologies. Please provide feedback on topologies being used in the wild to assist us with this.

Changes:

  • Extracted some core types into their own schema
  • Added a new Account type which represents an account on the connector. It may wrap a plugin but may also simply be a proxy to another connector.
  • New Account objects are provided by pluggable AccountProvider objects. The default implementation is being the PluginAccountProvider which instantiates a new Account for each plugin in the configuration or in response to app.addPlugin().
  • A connector running a ServerAccountProvider is intended to replace the various "asym-server" plugins in use today based on mini-accounts. This provider listens on a single port for new clients and then creates a new Account` for each incoming connection.
  • Added configuration profiles which allow the app to be run in connector or plugin mode. In plugin mode the connector behaves like a stand-alone plugin hosting just that one plugin and the middleware configured for it. The connector will have a single uplink to another connector and will not have any routing functionality.
  • Removed support for the v1 plugins (this was pulling in a LOT of legacy code)
  • Modified middleware to pass deserialised packets down the pipeline (i.e. packets are deserialised and serialised once.)
  • Modified package structure to be closer to other TypeScript projects
  • Added load testing via a JMeter Docker container (to be published)

Notes:

The new architecture has resulted in some optimisations which need to be thoroughly tested. One of these is that the app handles new accounts asynchronously. When the Account is created it is passed to the app which then calls Account.startup() before starting to track the account in the routing service and reloading the routing tables. This can mean there is a small delay between adding an account and being able to route messages to that account.

adrianhopebailie added some commits Nov 22, 2018

chore: clean up project structure to match other TSC projects
refactor: rm serialization, change middleware
feat: create pluggable account manager and account service. This allows to switch between using in/out of process plugins.
fix: add and remove handlers for on connection and removal of connection
fix: change the manner in which loading own ILP Address
fix: IlpReply and isFulfill to be imported from ilp-packet module
refactor: wire up middleware pipeline in new account handler
* remove accounts dependency in middleware manager
refactor: remove core as a dep for accounts
refactor: clean up and use DI correctly
refactor: account service decides on new accounts middleware
refactor: plugin mode
* added config validation
* Added some test coverage

feat(docker): fix docker file and add examples

* Fixed the dockerfile to compile the connectors src files
* Added some documentation and examples to running connector in different modes

fix: schemas, generalize server provider

refactor: update server-provider tests

fix: merging of default account values with user set ones

refactor: remove deprecated server mode tests

refactor: plugin mode requires account named parent

refactor(profiles): update tests to use new config setup

refactor: update admin api tests

feat: load-test utilities

* add loop-back account and service provider
* add load-test directory with default load test configuration

adrianhopebailie and others added some commits Dec 7, 2018

fix: add delays for some failing tests
load error middleware in correct position
fix(tests): fix tests and linting
* Fixed some tests failing due to environment variables
* Fixed linting errors
* Make load test pull in image from dockerhub
fix: update circleci script
* update circleci script to use correct code coverage script
@codecov-io

This comment has been minimized.

Copy link

commented Dec 10, 2018

Codecov Report

Merging #476 into master will decrease coverage by 0.24%.
The diff coverage is 74.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
- Coverage   73.62%   73.37%   -0.25%     
==========================================
  Files          44       53       +9     
  Lines        2059     2235     +176     
  Branches      332      348      +16     
==========================================
+ Hits         1516     1640     +124     
- Misses        543      595      +52
Impacted Files Coverage Δ
src/backends/ecb.ts 59.09% <0%> (ø) ⬆️
src/common/log.ts 100% <100%> (ø) ⬆️
src/middlewares/stats.ts 100% <100%> (+31.25%) ⬆️
src/services/route-builder.ts 90.38% <100%> (ø) ⬆️
src/middlewares/error-handler.ts 94.11% <100%> (+29.83%) ⬆️
src/middlewares/expire.ts 100% <100%> (+5.26%) ⬆️
src/types/account-provider.ts 100% <100%> (ø)
src/middlewares/max-packet-amount.ts 100% <100%> (+4.34%) ⬆️
src/services/routing-table.ts 100% <100%> (ø) ⬆️
src/services/rate-backend.ts 100% <100%> (ø) ⬆️
... and 48 more

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 a7c4bcd...59efb2e. Read the comment docs.

@matdehaast

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

We also need to do a check with moneyd and this new branch

matdehaast added some commits Dec 10, 2018

fix: integration tests
* fix bug in inheriting ILP address from parent
* update circleci script for integration tests
fix: integration tests
* move the mocha exit to package.json
@sentientwaffle
Copy link
Contributor

left a comment

wip review: mostly nits and some questions so far

@@ -16,39 +16,43 @@
## Table of Contents

* [Overview](#overview)
* * [Overview](#overview)

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

Remove double bullet list

* [Development](#development)
* [Configuration Variables](#configuration-variables)

* [| \*.throughput.refillPeriod | integer | Optional Length of time (in milliseconds) during which the token balance increases by incomingAmount/outgoingAmount tokens. Defaults to one second. |](#-throughputrefillperiod----integer--------optional-length-of-time-in-milliseconds-during-which-the-token-balance-increases-by-incomingamountoutgoingamount-tokens-defaults-to-one-second-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------)

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

Paste error?

"bignumber.js": "^7.2.1",
"change-case": "^3.0.1",
"debug": "^3.1.0",
"@types/leveldown": "^4.0.0",

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

Shouldn't the @types/* be in devDependencies?

"start-prof": "node --prof --logfile=${CONNECTOR_V8_LOGFILE:-v8.log} src/index.js",
"start:watch": "nodemon src/index.js",
"build": "npm run schema-to-tsd && npm run compile-ts",
"clean": "rm -Rf .nyc_output && rm -Rf coverage && rm -Rf build ",

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

rm takes multiple directories

@@ -244,6 +529,8 @@ If there are multiple parents, and `ilpAddress` is not set explicit, specify the
| `*.throughput.outgoingAmount` | string | _Optional_ Maximum throughput amount (in atomic units; per second) for outgoing packets. If this setting is not set, the outgoing throughput limit is disabled. |
| `*.throughput.refillPeriod` | integer | _Optional_ Length of time (in milliseconds) during which the token balance increases by `incomingAmount`/`outgoingAmount` tokens. Defaults to one second. |

> > > > > > > master

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

merge debris?

}
})

pipelines.incomingMoney.insertLast({

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

Why was this removed?

this.plugin.sendData(serializeCcpRouteControlRequest(routeControl))
.then(data => {
if (data[0] === Type.TYPE_ILP_FULFILL) {
this.account.sendIlpPacket(deserializeIlpPrepare(serializeCcpRouteControlRequest(routeControl)))

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

ilp-protocol-ccp should be updated to support receiving/returning packet objects to prevent the extra deserialize/serialize calls.

const info = this.accounts.getInfo(account)
const accountId = match[1]
const account = this.accounts.get(accountId)
const plugin = (account as WrapperAccount).getPlugin()

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

If account is always a WrapperAccount, maybe accounts.get should return it as one?

}

exists (accountId: string) {
return this.accounts.has(accountId)
public registerCoreIlpPacketHandler (handler: (packet: IlpPrepare, accountId: string, outbound: (packet: IlpPrepare, accountId: string) => Promise<IlpReply>) => Promise<IlpReply>) {

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

as far as I can tell, registerCoreIlpPacketHandler and registerCoreMoneyHandler are only ever called once -- shouldn't the "handlers" just be methods? Or if they need to be handlers, the functions should just be constructor parameters.

try {
await this._handleNewAccount(account, provider)
} catch (e) {
log.error(`error handling new account: ${account.id}`)

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Dec 21, 2018

Contributor

If there's an error during provider.startup, should startup reject?

If not, at least be sure to log the error message

@@ -11,6 +11,12 @@
"enum": ["production", "test"],
"default": "test"
},
"profile": {
"description": "Determines the configuration defaults to use when running the connector. Can be: 'connector', 'server', 'plugin', 'cluster'. Default: 'connector'",

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

Can be: 'connector', 'server', 'plugin', 'cluster'.

Server isn't listed in the enum.


Also, the description neglects to mention that the profile also determines the handler core.ts uses for incoming packets.

@@ -11,6 +11,12 @@
"enum": ["production", "test"],
"default": "test"
},
"profile": {

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

Are the different profiles documented anywhere?

@@ -496,7 +495,12 @@ export default class RouteBroadcaster {
epoch
}

this.forwardingRoutingTable.insert(prefix, routeUpdate)
// this.forwardingRoutingTable.insert(prefix, routeUpdate)
if (typeof newNextHop === 'string') {

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

👍 for the route broadcaster fix, but could this be moved to a separate commit & PR? It's getting lost in the noise of the refactor.


export interface MiddlewareDefinition {
type: string,
options?: object
}

/**
* Services the connector exposes to middleware.
*/

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

Why delete this comment?

const logger = require('../../build/common/log')
const LoopBackAccountProvider = require('../../build/account-providers/loop-back').default

describe('server provider', function () {

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

wrong description string

if (!account) {
log.error('could not find plugin for account id. accountId=%s', accountId)
throw new Error('unknown account id. accountId=' + accountId)
throw new Error('unable to send money. unknown account: ' + accountId)

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

this.get(accountId) will throw if the account doesn't exist, so this branch is unreachable.

}

export function constructAccountProvider (config: AccountProviderConfig, deps: reduct.Injector): AccountProvider {
const AccountServiceProviderConst = loadModuleOfType('account-provider', config.type) as AccountProviderConstructor

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

I think dynamically require()-ing modules is brittle. If there are problems with the injected modules, it would be better to discover them at compile time. Suggestions:

  • Remove require()-ing packages by string for accounts and middlewares. The latter shouldn't be breaking: afaik no one is using custom connector middlewares yet.
  • Consider deprecating it for plugin loading.

Custom accounts/middlewares/plugins are an advanced feature. To use them, users should need to write a wrapper module that requires and injects them into connector.createApp().

This enables typechecking if the modules are written in typescript. Also, it is a step towards allowing the connector to be run in the browser.

],
"additionalProperties": false
}
"additionalProperties": { "$ref": "AccountConfig.json" }

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

untested:

Suggested change
"additionalProperties": { "$ref": "AccountConfig.json" }
"$ref": "AccountConfig.json"

(applies to several places in the schema)

"profile": {
"description": "Determines the configuration defaults to use when running the connector. Can be: 'connector', 'server', 'plugin', 'cluster'. Default: 'connector'",
"type": "string",
"enum": ["connector", "plugin", "cluster"],

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

They're all connector profiles, so "connector" is not very descriptive.

async function shutdown (
accounts: Accounts,
routeBroadcaster: RouteBroadcaster
) {

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Jan 14, 2019

Contributor

Shouldn't this tear down the providers, their accounts, disconnect plugins, etc?

@sharafian

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Can this be closed? Seems like it was superseded by rafiki

@adrianhopebailie

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

These ideas were incorporated into Rafiki: https://medium.com/interledger-blog/introducing-rafiki-e3de4710d3de

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.