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

feat\vault: use mock quic-p2p from routing #901

Merged
merged 1 commit into from Dec 4, 2019

Conversation

@maqi
Copy link
Member

maqi commented Nov 29, 2019

No description provided.

@maqi maqi requested a review from ustulation as a code owner Nov 29, 2019
@maqi maqi requested review from nbaksalyar, jeanphilippeD and octol Nov 29, 2019
Copy link
Contributor

jeanphilippeD left a comment

Some comments

@@ -58,31 +55,9 @@ impl Environment {
pub fn with_multiple_vaults(num_vaults: usize) -> Self {
assert!(num_vaults > 0);

let do_format = move |formatter: &mut Formatter, record: &Record<'_>| {

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 3, 2019

Contributor

Why did that change for this PR?

This comment has been minimized.

Copy link
@maqi

maqi Dec 3, 2019

Author Member

We can now use the Network from routing directly, and doesn't require this format anymore.

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 3, 2019

Contributor

Isn't it for log? isn't it up to vault how to decide how to format things?

This comment has been minimized.

Copy link
@maqi

maqi Dec 3, 2019

Author Member

the logbuilder here conflicts with the log initializer inside the routing :(

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 3, 2019

Contributor

Ok, not ideal, probably still needed when using mock_routing though?

This comment has been minimized.

Copy link
@maqi

maqi Dec 3, 2019

Author Member

probably still needed when using mock_routing though

vault::mock_routing is using routing::mock_quick_p2p which has the log initializer.

Is there a way it would not conflict?

make routing::mock_quick_p2p using the log builder from vault? however I think it will make the current API exposing more stuff.

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 3, 2019

Contributor

Ok, make sense, if @nbaksalyar is happy, I'm ok with it.

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Dec 3, 2019

Member

make routing::mock_quick_p2p using the log builder from vault?

Maybe the other way around, @maqi? That is, instead of exposing & defining the logger in mock-quic-p2p directly, initialise it only at the use site, not in the library code. In this case the use site is safe_vault::tests::Environment, and in Routing I guess we can have something similar and initialise the logger in the Routing tests (within a wrapper, if needed)?

should we remove this or should it be used, and if it should be used what should it do.

I agree this is not ideal. We should decouple test logs from Routing. Currently, routing depends on maidsafe_utilities which is deprecated, so eventually it needs to be replaced there too.

This comment has been minimized.

Copy link
@maqi

maqi Dec 3, 2019

Author Member

hmm... the change will then requires the log initializer to be separated from the current routing::mock_quick_p2p::network constructor. :(

I'd prefer it to be handled as separate issue/PR ?

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Dec 4, 2019

Member

Yeah, separate issue/PR is OK 👍

x => unexpected!(x),
let max_fetches = 10;
let mut steps = 0;
while steps < max_fetches {

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 3, 2019

Contributor

Why did that change for this PR?

This comment has been minimized.

Copy link
@maqi

maqi Dec 3, 2019

Author Member

there is new event SentUserMessage defined in routing, which will be received during normal communication and shall be ignored here

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 3, 2019

Contributor

Then why do we have a max_fetches and steps? Shouldn't we loop until either no more events are there or we get the one we look for?

x => unexpected!(x),
let max_fetches = 10;
let mut steps = 0;
while steps < max_fetches {

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 3, 2019

Contributor

same?

This comment has been minimized.

Copy link
@maqi

maqi Dec 3, 2019

Author Member

yeah, same reason.
there is new event SentUserMessage defined in routing, which will be received during normal communication and shall be ignored here

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 3, 2019

Contributor

same

@maqi maqi force-pushed the maqi:issue_895_quic-p2p_replace branch 2 times, most recently from 13cac7c to b44f35d Dec 3, 2019
Copy link
Member

nbaksalyar left a comment

Looks great! 👍 Just 1 comment about logging.

@@ -58,31 +55,9 @@ impl Environment {
pub fn with_multiple_vaults(num_vaults: usize) -> Self {
assert!(num_vaults > 0);

let do_format = move |formatter: &mut Formatter, record: &Record<'_>| {

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Dec 3, 2019

Member

make routing::mock_quick_p2p using the log builder from vault?

Maybe the other way around, @maqi? That is, instead of exposing & defining the logger in mock-quic-p2p directly, initialise it only at the use site, not in the library code. In this case the use site is safe_vault::tests::Environment, and in Routing I guess we can have something similar and initialise the logger in the Routing tests (within a wrapper, if needed)?

should we remove this or should it be used, and if it should be used what should it do.

I agree this is not ideal. We should decouple test logs from Routing. Currently, routing depends on maidsafe_utilities which is deprecated, so eventually it needs to be replaced there too.

@maqi maqi force-pushed the maqi:issue_895_quic-p2p_replace branch from b44f35d to 22ac48d Dec 3, 2019
Copy link
Member

nbaksalyar left a comment

LGTM 👍

@nbaksalyar nbaksalyar merged commit cddc633 into maidsafe:vNext Dec 4, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
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.