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

fix: pow issue #214

Merged
merged 2 commits into from
Jun 30, 2024
Merged

fix: pow issue #214

merged 2 commits into from
Jun 30, 2024

Conversation

hunjixin
Copy link
Contributor

Review Type Requested (choose one):

  • Glance - superficial check (from domain experts)
  • Logic - thorough check (from everybody doing review)
  1. revert bacalhau id
  2. remove parse event

Summary

Provide a one line summary and link to any relevant references

Task/Issue reference

Closes: add_link_here

Details (optional)

Add any additional details that might help Code Reviewers digest this PR

How to test this code? (optional)

Anything else? (optional)

@hunjixin hunjixin changed the title Fix/pow issue fix: pow issue Jun 28, 2024
@github-actions github-actions bot added the fix label Jun 28, 2024
runOutput := splitOutputs[len(splitOutputs)-1]
outputError := strings.Join(strings.Fields(strings.Join(splitOutputs[:len(splitOutputs)-1], " ")), " ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you removing this output? We cannot just silently disregard a potential error message in the combined output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the id command ouput
udp buf size error log
result json.

this code not work.
maybe we can check by line number,if lines more than one. give a warn log. do you think it okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

How can we fix the udp buf size error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

udp buf eeror occur in bacalau id command. i dont know how to fix.
maybe change some system config can fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, capture the id, disregard the rest.

Copy link
Contributor

Choose a reason for hiding this comment

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

for the udp buffer error, this is from bacalhau document:

While executing this command, you may encounter warnings regarding receive and send buffer sizes: failed to sufficiently increase receive buffer size. These warnings can arise due to limitations in the UDP buffer used by Bacalhau to process tasks. Additional information can be found in https://github.com/quic-go/quic-go/wiki/UDP-Buffer-Sizes.

I tested it and it works!

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this too and it seems to work.

I think it's ok to discard the error output here because Bacalhau seems to be including irrelevant error output in the id command output. That's weird behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we should change to use rpc instead of subprocess command.

#195

Copy link
Contributor

@richardbremner richardbremner left a comment

Choose a reason for hiding this comment

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

lgtm

runOutput := splitOutputs[len(splitOutputs)-1]
outputError := strings.Join(strings.Fields(strings.Join(splitOutputs[:len(splitOutputs)-1], " ")), " ")
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this too and it seems to work.

I think it's ok to discard the error output here because Bacalhau seems to be including irrelevant error output in the id command output. That's weird behavior.

@richardbremner richardbremner merged commit c9fb71a into main Jun 30, 2024
2 of 3 checks passed
@richardbremner richardbremner deleted the fix/pow_issue branch June 30, 2024 23:44
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

4 participants