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.

Sign up for GitHub

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

Jump to bottom

6th step - remove output key from execution #975

Merged
merged 3 commits into from
May 23, 2019
Merged

6th step - remove output key from execution #975

merged 3 commits into from
May 23, 2019

Conversation

krhubert
Copy link
Contributor

No description provided.

NicolasMahe
NicolasMahe reviewed May 22, 2019
View reviewed changes
api/api.go Outdated Show resolved Hide resolved
@mesg-bot
Copy link
Member

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/simplification-of-the-tasks-output/265/6

antho1404
antho1404 approved these changes May 22, 2019
View reviewed changes
interface/grpc/service/service.go
@@ -69,5 +70,8 @@ func (s *Server) ListenTask(request *serviceapi.ListenTaskRequest, stream servic

// SubmitResult submits results of an execution.
func (s *Server) SubmitResult(context context.Context, request *serviceapi.SubmitResultRequest) (*serviceapi.SubmitResultReply, error) {
return &serviceapi.SubmitResultReply{}, s.api.SubmitResult(request.ExecutionID, request.OutputKey, []byte(request.OutputData))
if request.OutputKey == "error" {
return &serviceapi.SubmitResultReply{}, s.api.SubmitResult(request.ExecutionID, nil, errors.New(request.OutputData))
Copy link
Member

Choose a reason for hiding this comment

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

should be request.OutputData.message, the error here will look like
"{message: 'xxx'}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not everyone supposes to put an error in the message. Here error needs to be just string. If you want to extend it somehow that's fine, but at the base is a string.

Why do we need to force message here? what's the point if we can have just plain text?

antho1404
antho1404 requested changes May 22, 2019
View reviewed changes
Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

don't have the result when I execute a task that returns an error

@krhubert krhubert requested a review from antho1404 May 22, 2019 14:14
@krhubert
Copy link
Contributor Author

There is the bug in core :

// ListenResult listens for results from a services.
func (s *Server) ListenResult(request *coreapi.ListenResultRequest, stream coreapi.Core_ListenResultServer) error {
  filter := &api.ExecutionFilter{
    Status:  execution.Completed,
    TaskKey: request.TaskFilter,
    Tags:    request.TagFilters,
  }

So the ListenResult won't listen the exeuction that failed, it's not related to this PR

@krhubert krhubert requested a review from NicolasMahe May 22, 2019 14:27
@antho1404
Copy link
Member

Here the execution changes state so we should have it on the stream, this is a regression

engine/interface/grpc/core/core.go

Line 180 in 60540cf

Error: execution.Error,

but ok this can be fixed in parallel but needs to be fixed

@NicolasMahe NicolasMahe changed the title 6th step - remove output key from exeuction 6th step - remove output key from execution May 22, 2019
@mesg-bot
Copy link
Member

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/simplification-of-the-tasks-output/265/7

antho1404
antho1404 approved these changes May 23, 2019
View reviewed changes
Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

I guess all issues on this one are fixed in the next ones, @krhubert if it's right you can merge this one

@NicolasMahe NicolasMahe merged commit 45d723b into feature/single-output May 23, 2019
@NicolasMahe NicolasMahe deleted the feature/single-output-pt-6 branch May 23, 2019 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasMahe NicolasMahe NicolasMahe approved these changes

@antho1404 antho1404 antho1404 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@krhubert @mesg-bot @antho1404 @NicolasMahe