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

Extend error msg to specify operation name for missing index #872

Merged
merged 3 commits into from
Jan 20, 2020
Merged

Extend error msg to specify operation name for missing index #872

merged 3 commits into from
Jan 20, 2020

Conversation

ebadyano
Copy link
Contributor

Closes #597

@ebadyano ebadyano added enhancement Improves the status quo :Usability Makes Rally easier to use labels Jan 15, 2020
@ebadyano ebadyano added this to the 1.4.1 milestone Jan 15, 2020
@ebadyano ebadyano self-assigned this Jan 15, 2020
danielmitterdorfer
danielmitterdorfer reviewed Jan 16, 2020
View reviewed changes
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

I left a couple of suggestions. Can you please also add a test for SearchParamSource that raises the error?

esrally/track/params.py Outdated
@@ -365,7 +365,7 @@ def __init__(self, track, params, **kwargs):
}

if not index_name:
raise exceptions.InvalidSyntax("'index' is mandatory")
raise exceptions.InvalidSyntax("'index' is mandatory and is missing for operation '%s'" % kwargs.get("operation_name"))
Copy link
Member

Choose a reason for hiding this comment

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

As per convention we should use the format method instead of % formatting.

esrally/track/loader.py Outdated
@@ -314,7 +314,7 @@ def operation_parameters(t, op):
if op.param_source:
return params.param_source_for_name(op.param_source, t, op.params)
else:
return params.param_source_for_operation(op.type, t, op.params)
return params.param_source_for_operation(op.type, t, op.params, op.name)
Copy link
Member

Choose a reason for hiding this comment

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

Each operation will have a name but if we specify operations inline with a task, it's the operation type's name. How about we instead use the task's name here which is always recognizable by the user (see attached patch for a sketch).

@danielmitterdorfer
Copy link
Member

@ebadyano seems make precommit has failed on the PR build:


15:01:15 + make precommit
15:01:23 ************* Module tests.track.params_test
15:01:23 tests/track/params_test.py:1606:0: C0303: Trailing whitespace (trailing-whitespace)

Can you please have a look?

danielmitterdorfer
danielmitterdorfer approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@ebadyano ebadyano merged commit b83c9fa into master Jan 20, 2020
@ebadyano ebadyano deleted the track_preparator_error_msg branch May 13, 2020 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielmitterdorfer danielmitterdorfer danielmitterdorfer approved these changes

Assignees

@ebadyano ebadyano

Labels
enhancement Improves the status quo :Usability Makes Rally easier to use
Projects
None yet
Milestone
1.4.1
Development

Successfully merging this pull request may close these issues.

Improve error message when no index has been specified in search operation with multiple indices
2 participants
@ebadyano @danielmitterdorfer