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

Make Redis fully optional for ilp-node #551

Merged
merged 3 commits into from Dec 12, 2019
Merged

Make Redis fully optional for ilp-node #551

merged 3 commits into from Dec 12, 2019

Conversation

@bstrie
Copy link
Collaborator

bstrie commented Dec 4, 2019

This should be the final part of laying the groundwork for alternative backing databases.

@bstrie bstrie requested review from emschwartz and gakonst Dec 4, 2019
@bstrie bstrie requested a review from dora-gt as a code owner Dec 4, 2019
@gakonst

This comment has been minimized.

Copy link
Member

gakonst commented Dec 4, 2019

Do you think it'd be possible to abstract the E2E tests such that they run once with each available backend? So instead of putting everythign under ilp-node/tests/redis, we'd use ilp-node/tests (as is now), and then the test would:

  1. import all backends
  2. put them in a trait object vector [trait probably would be NodeStore ])
  3. run all tests for that backend (launch backend, launch node and plug it on the backend's driver, kill node and backend, move on to the next one)
@bstrie

This comment has been minimized.

Copy link
Collaborator Author

bstrie commented Dec 4, 2019

Do you think it'd be possible to abstract the E2E tests such that they run once with each available backend?

Yes, in the long run I would like to share these tests between all the storage backends, since otherwise there would be a substantial amount of duplication. For now though I was planning on doing that later as part of the SQLite backend effort, once it's mature enough to start needing tests. At that point I intend to extract the generic bits out of the tests/redis directory, but since none of it's generic today (and I think that's lower priority than working on the SQLite store itself) I just shoved all of the tests into the redis-specific directory for the time being.

crates/ilp-node/src/main.rs Outdated Show resolved Hide resolved
crates/ilp-node/src/node.rs Outdated Show resolved Hide resolved
crates/ilp-node/src/node.rs Show resolved Hide resolved
crates/ilp-node/src/node.rs Show resolved Hide resolved
crates/ilp-node/src/node.rs Outdated Show resolved Hide resolved
crates/ilp-node/src/redis_store.rs Show resolved Hide resolved

#[doc(hidden)]
#[allow(dead_code)]
pub fn insert_account_with_redis_store(

This comment has been minimized.

Copy link
@emschwartz

emschwartz Dec 4, 2019

Member

Is this only used in the tests?

This comment has been minimized.

Copy link
@bstrie

bstrie Dec 4, 2019

Author Collaborator

This isn't used at all in the tests in this repo, but the settlement-engines repo does call this function from its tests. Interestingly, in those tests is the comment // TODO insert the accounts via HTTP request ( https://github.com/interledger-rs/settlement-engines/blob/d8995867751ee62b13bbc4a785588a1a30bff62a/crates/ethereum-engine/tests/eth_ledger_settlement.rs#L85 ), which implies that this function could be removed entirely. @gakonst , how difficult would that be? Does this code simply predate the ability to insert accounts via the HTTP API?

This comment has been minimized.

Copy link
@emschwartz

emschwartz Dec 4, 2019

Member

Does this code simply predate the ability to insert accounts via the HTTP API?

Yes. The tests should be changed to use the CLI or the HTTP API directly

This comment has been minimized.

Copy link
@bstrie

bstrie Dec 5, 2019

Author Collaborator

Since updating settlement-engines seems out of scope at the moment, for now I've just marked this function with the deprecated attribute so that it's less likely to be forgotten about.

This comment has been minimized.

Copy link
@gakonst

gakonst Dec 5, 2019

Member

@bstrie Thanks, this is code which existed before we had integrated adding accounts only via the HTTP API. You can safely remove it, and I will add a PR to settlement-engines which converts the E2E tests to not use it anymore. Not difficult at all.

crates/ilp-node/tests/redis/accounts_api.rs Outdated Show resolved Hide resolved
crates/ilp-node/tests/redis/accounts_api.rs Outdated Show resolved Hide resolved
@bstrie bstrie force-pushed the bs-unredis branch from 2c0ebca to f218712 Dec 5, 2019
@bstrie bstrie changed the title [Draft] Make Redis fully optional for ilp-node Make Redis fully optional for ilp-node Dec 5, 2019
@bstrie

This comment has been minimized.

Copy link
Collaborator Author

bstrie commented Dec 5, 2019

Changes addressed and pushed. Note that this PR contains a breaking change to the command-line invocation of ilp-node, as the "redis_url" option has been renamed to "database_url". @dora-gt , do you think this might cause any CI problems? Specifically, I imagine there could be a problem if a new script (using "database_url") somehow gets used to call an old binary (expecting "redis_url"). We could theoretically address this quite easily (search for "XXX" in the PR diff to see what would need to change in order to temporarily support old binaries with new scripts), but I'm not sure whether anybody else thinks this is worth worrying about.

@bstrie

This comment has been minimized.

Copy link
Collaborator Author

bstrie commented Dec 5, 2019

I think the CI failures seen here may be what I was just suggesting in the prior comment. I'll push a version with the aliases and we'll see how CI likes that. @emschwartz , you mentioned that if this happened then you'd want to consider changes to how the CI works?

@bstrie bstrie force-pushed the bs-unredis branch from fedc5cd to ee701dd Dec 5, 2019
@bstrie

This comment has been minimized.

Copy link
Collaborator Author

bstrie commented Dec 5, 2019

Rebased.

@bstrie bstrie requested a review from emschwartz Dec 5, 2019
@bstrie bstrie force-pushed the bs-unredis branch from ee701dd to 04b21c7 Dec 6, 2019
crates/ilp-node/src/redis_store.rs Outdated Show resolved Hide resolved
crates/ilp-node/src/node.rs Outdated Show resolved Hide resolved
crates/ilp-node/src/node.rs Outdated Show resolved Hide resolved
BREAKING CHANGE: the "redis_url" command-line option to the ilp-node binary
has been renamed to "database_url"
@bstrie bstrie force-pushed the bs-unredis branch from 04b21c7 to ad6edd3 Dec 12, 2019
@bstrie bstrie merged commit e3cb7d1 into master Dec 12, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-md Your tests passed on CircleCI!
Details
@bstrie bstrie deleted the bs-unredis branch Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.