Skip to content

Error in execution result#651

Merged
antho1404 merged 7 commits intodevfrom
feature/execution-result-error
Dec 22, 2018
Merged

Error in execution result#651
antho1404 merged 7 commits intodevfrom
feature/execution-result-error

Conversation

@NicolasMahe
Copy link
Copy Markdown
Member

@NicolasMahe NicolasMahe commented Dec 18, 2018

close #610. close #573.
simplified version of #596 (close #596)

I change the big modif on the proto def from #596 to just add string error.

@NicolasMahe NicolasMahe requested review from antho1404, ilgooz and krhubert and removed request for antho1404 and ilgooz December 18, 2018 12:28
Comment thread execution/execution.go
Copy link
Copy Markdown
Contributor

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

Comment thread execution/execution_test.go Outdated
Comment thread execution/execution_test.go Outdated
Comment thread execution/execution_test.go Outdated
Comment thread commands/service_dev.go Outdated
Comment thread commands/service_dev.go Outdated
Comment thread commands/service_dev.go
Comment thread api/submit_result.go
Comment thread api/submit_result.go
if err := exec.Complete(outputKey, outputData); err != nil {
return err

if err := exec.Complete(outputKey, outputDataMap); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but we should also save the failed messages.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

can you elaborate, i don't understand

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An invalid json response we should also be kept inside db. Someone may want to see the task response (even if it's invalid). I'm even thinking about passing this response, not just an error. But it just a thought and needs lot way thinking if this approach is valid.

@NicolasMahe
Copy link
Copy Markdown
Member Author

@mesg-foundation/core ready for reviews

ilgooz
ilgooz previously approved these changes Dec 19, 2018
Comment thread execution/execution_test.go Outdated
@NicolasMahe
Copy link
Copy Markdown
Member Author

@mesg-foundation/core ready for reviews.
The fix on the race condition will be done on: #652

krhubert
krhubert previously approved these changes Dec 20, 2018
@NicolasMahe
Copy link
Copy Markdown
Member Author

NicolasMahe commented Dec 21, 2018

I will fix the conflict after approval

ilgooz
ilgooz previously approved these changes Dec 21, 2018
antho1404
antho1404 previously approved these changes Dec 21, 2018
@antho1404 antho1404 dismissed stale reviews from ilgooz and themself via 54714c0 December 22, 2018 05:35
@antho1404 antho1404 force-pushed the feature/execution-result-error branch from 40164d6 to 54714c0 Compare December 22, 2018 05:35
@antho1404 antho1404 merged commit 02b8f62 into dev Dec 22, 2018
@NicolasMahe NicolasMahe deleted the feature/execution-result-error branch December 22, 2018 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Core shouldn't allow multiple output for the same execution service execute command hangs forever on unexpected data type

4 participants