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

Reestablish conn with mixnode if err occurs #501

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

youngjoon-lee
Copy link
Contributor

@youngjoon-lee youngjoon-lee commented Nov 2, 2023

I'm reopening the PR #445, now that CI (integration test) is stable.

99% is the same as the PR #445, but I added an one-line commit: 3414c58. Please double check this, @al8n.

Since #445 has been already reviewed, I didn't assign many reviewers. But, pls feel free to have a look.

@youngjoon-lee youngjoon-lee added this to the Mixnet milestone Nov 2, 2023
@youngjoon-lee youngjoon-lee self-assigned this Nov 2, 2023
Copy link
Contributor

@al8n al8n left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM!

@youngjoon-lee
Copy link
Contributor Author

Thanks! LGTM!

Another CI failure appears :) I'll take a look

@youngjoon-lee
Copy link
Contributor Author

youngjoon-lee commented Nov 3, 2023

Thanks! LGTM!

Another CI failure appears :) I'll take a look

It's still hard to debug since we cannot see logs from nodes, but CI seems to be failed because CI was busy at that time (similar as #494 and #492). I think this PR didn't affect to the CI failure because this PR is very simple and doesn't change the normal behavior.
I'll re-run CI multple times, and if CI doesn't fail with the same error, I'll merge this PR.

After all, I'll open another PR which helps us to see logs from nodes when CI integration tests fail.

@danielSanchezQ
Copy link
Collaborator

Thanks! LGTM!

Another CI failure appears :) I'll take a look

It's still hard to debug since we cannot see logs from nodes, but CI seems to be failed because CI was busy at that time (similar as #494 and #492). I think this PR didn't affect to the CI failure because this PR is very simple and doesn't change the normal behavior. I'll re-run CI multple times, and if CI doesn't fail with the same error, I'll merge this PR.

After all, I'll open another PR which helps us to see logs from nodes when CI integration tests fail.

So, looks like issue is in windows right?

Why windows why???

@youngjoon-lee
Copy link
Contributor Author

So, looks like issue is in windows right?

Why windows why???

It seems it's not only in windows. Codecov in ubuntu also fails with the similar (but maybe different) symptom. I'm trying to debug it again before merging this.

@youngjoon-lee
Copy link
Contributor Author

youngjoon-lee commented Nov 6, 2023

So, looks like issue is in windows right?
Why windows why???

It seems it's not only in windows. Codecov in ubuntu also fails with the similar (but maybe different) symptom. I'm trying to debug it again before merging this.

I've found that CIs on Windows are 2~3x slower than Ubuntu for some reason, even for the master branch: https://github.com/logos-co/nomos-node/actions/runs/6743301131/job/18330953687. So, I think the CI failures of this PR aren't related to the changes of this PR. We'll be able to be more confident about it with PR #508 soon (If any unexpected error isn't found in the logs, we could say the failure is due to the slow machine).

@youngjoon-lee youngjoon-lee changed the base branch from master to gh-artifacts November 6, 2023 05:20
@danielSanchezQ
Copy link
Collaborator

So, looks like issue is in windows right?
Why windows why???

It seems it's not only in windows. Codecov in ubuntu also fails with the similar (but maybe different) symptom. I'm trying to debug it again before merging this.

I've found that CIs on Windows are 2~3x slower than Ubuntu for some reason, even for the master branch: https://github.com/logos-co/nomos-node/actions/runs/6743301131/job/18330953687. So, I think the CI failures of this PR aren't related to the changes of this PR. We'll be able to be more confident about it with PR #508 soon (If any unexpected error isn't found in the logs, we could say the failure is due to the slow machine).

Sounds good. I will try locally as I have windows myself and post results here.

@danielSanchezQ
Copy link
Collaborator

Windows tests succeed locally. So probably something going around the gh runner.

Base automatically changed from gh-artifacts to master November 7, 2023 00:38
@youngjoon-lee youngjoon-lee merged commit 643bcef into master Nov 28, 2023
7 checks passed
@youngjoon-lee youngjoon-lee deleted the reestablish-conn-fixed branch November 28, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants