Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

n3h mode default to REAL #1282

Merged
merged 13 commits into from
May 21, 2019
Merged

n3h mode default to REAL #1282

merged 13 commits into from
May 21, 2019

Conversation

lucksus
Copy link
Collaborator

@lucksus lucksus commented Apr 17, 2019

PR summary

As discussed with @ddd-mtl, the n3h mode to use as default should be REAL now.

changelog

Please check one of the following, relating to the CHANGELOG

  • this is a code change that effects some consumer (e.g. zome developers) of holochain core so there is an 'Unreleased' CHANGELOG item, with the format - summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)
  • this is not a code change, or doesn't effect anyone outside holochain core development

Copy link
Contributor

@neonphog neonphog left a comment

Choose a reason for hiding this comment

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

👍

CHANGELOG.md Outdated
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed

- Increased timeout on n3h spawn and wait for `#P2P-READY#` message [#1276](https://github.com/holochain/holochain-rust/pull/1276).
- Default N3H mode as set when spawned by the conductor got set to "REAL". [#1282](https://github.com/holochain/holochain-rust/pull/1282)
Copy link
Collaborator

@Connoropolous Connoropolous Apr 17, 2019

Choose a reason for hiding this comment

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

Could we take this opportunity to clarify internally and externally what these modes actually are? I personally don’t even know, in clear terms

Copy link
Contributor

Choose a reason for hiding this comment

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

REAL is a big update to HACK mode.
HACK is going to dissapear shortly.
Consider REAL to be HACK mode's new name.
I needed to keep both around to make sure things didn't break.

REAL is in opposition to MOCK mode.
Mock mode is where you connect multiple local nodes to one n3h in order to simulate a network over IPC.
REAL is the normal n3h mode that gossips over the network.
Maybe renamed it to NORMAL?

Is that clear enough for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep

Copy link
Contributor

Choose a reason for hiding this comment

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

@Connoropolous @ddd-mtl can it be a small followup to do a rename and document this?

Copy link
Contributor

@thedavidmeister thedavidmeister left a comment

Choose a reason for hiding this comment

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

document values now or later/followup?

/// MOCK is for using n3h only as a local hub that apps connect to directly, i.e. n3h will
/// not connect to any other n3h instance.
/// HACK is for trying out things and should only be used by n3h developers who know why
/// they need this (i.e. testing new code). Might get removed soon.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ddd-mtl, is this correct?
@Connoropolous, is this sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, except for HACK. I'd write:

/// HACK - Deprecated. Used by n3h developers only. Will get removed soon.

@lucksus
Copy link
Collaborator Author

lucksus commented Apr 23, 2019

@thedavidmeister, @Connoropolous, @ddd-mtl, ok to merge?

Copy link
Collaborator

@Connoropolous Connoropolous left a comment

Choose a reason for hiding this comment

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

I'm an approve and since Davids change request was only regarding documentation, which is now addressed, I propose you can dismiss the review and go ahead

@lucksus lucksus changed the title n3h mode default to REAL WIP: n3h mode default to REAL Apr 26, 2019
@lucksus
Copy link
Collaborator Author

lucksus commented Apr 26, 2019

I‘ve turned this into a WIP since REAL Mode seems to have problems still while HACK works. Let us merge this after REAL mode got fixed.

@ddd-mtl
Copy link
Contributor

ddd-mtl commented Apr 26, 2019

@lucksus Agree on postponing this.
On my side, waiting on Core to do integration test and isolating the hypothetical problem before looking into a potential fix.

@lucksus
Copy link
Collaborator Author

lucksus commented Apr 26, 2019

@ddd-mtl, did we not identify conceptual issues that will take ~ 2 weeks solving anyway? Shouldn't we start working on that asap? @neonphog?

@lucksus
Copy link
Collaborator Author

lucksus commented Apr 26, 2019

Also, until we have core <> n3h integration tests we can only manually run apps to test integration as I did so far with holochain-basic-chat. But that is a possibility in the now. I stopped doing that for solving the issues in REAL mode because HACK mode seems to be working and sufficient for getting to working apps via the internet as soon as we solve the IPv6/zerotier issues..

@ddd-mtl
Copy link
Contributor

ddd-mtl commented Apr 26, 2019

@lucksus Yes, the other issue is n3h stores entries & metas without waiting for Core to validate it. This is not blocking so not a priority.

@Connoropolous Connoropolous added WIP work in progress and removed WIP work in progress labels May 1, 2019
@lucksus lucksus changed the title WIP: n3h mode default to REAL n3h mode default to REAL May 21, 2019
@lucksus lucksus removed the WIP work in progress label May 21, 2019
@lucksus lucksus merged commit 412c926 into develop May 21, 2019
@zippy zippy deleted the n3h-default-real branch October 4, 2019 18:34
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.

6 participants