Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Persistent nodes #332

Merged
merged 20 commits into from
Feb 24, 2020
Merged

Persistent nodes #332

merged 20 commits into from
Feb 24, 2020

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Feb 17, 2020

Here is the draft PR adding supporting for persistence of node information.

  • a test with 2 different PersistentNetworkNodes using the same backend, showing one sends the info to the other.

Note I added env support for 2 config keys here. I will add all env support in a separate PR to reduce the mess.

Copy link
Member

@lucassaldanha lucassaldanha left a comment

Choose a reason for hiding this comment

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

PR is looking good! There are a few suggestions along the code.

One thing that I didn't see and I believe it is missing is the cli option that can be used to delete the known nodes data during startup. Let me know when this logic is implemented and I'll do another pass on the PR!

@atoulme
Copy link
Contributor Author

atoulme commented Feb 21, 2020

On second thought I removed the acceptance test. It would be more meaningful to have a test with a load balancer in place and using docker. I will add env variables to this PR to speed up the work.

@atoulme
Copy link
Contributor Author

atoulme commented Feb 21, 2020

Acceptance test passes locally. Please rerun the test?

lucassaldanha
lucassaldanha previously approved these changes Feb 23, 2020
Copy link
Member

@lucassaldanha lucassaldanha left a comment

Choose a reason for hiding this comment

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

LGMT. I've added a single comment about reuse.

src/main/java/net/consensys/orion/config/Config.java Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants