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

NoSQLBench workloads - auto token generation #358

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Conversation

jeffreyscarpenter
Copy link
Contributor

Update JSON API NoSQLBench workloads to take advantage of automatic token generation.

Also includes minor changes to emulate improvements made to Docs API NB workloads under nosqlbench/nosqlbench#1201

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@jeffreyscarpenter
Copy link
Contributor Author

This has been tested with locally compiled version of nb5, but should not be merged until the release of NB 5.17.3.

@jeffreyscarpenter
Copy link
Contributor Author

This has been tested with locally compiled version of nb5, but should not be merged until the release of NB 5.17.3.

Actually this is not correct. Tests pass with NB 5.17.2, and this can be approved and merged. https://github.com/stargate/jsonapi/actions/runs/4708744043

@jeffreyscarpenter jeffreyscarpenter marked this pull request as ready for review April 15, 2023 16:32
@jeffreyscarpenter jeffreyscarpenter requested a review from a team as a code owner April 15, 2023 16:32
Copy link
Contributor

@ivansenic ivansenic left a comment

Choose a reason for hiding this comment

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

The token is added only in the nosqlbench/http-jsonapi-crud-basic.yaml, other files don't have this change.. Will this be updated?


# nb5 -v run driver=http yaml=http-jsonapi-crud-basic jsonapi_host=my_jsonapi_host auth_token=$AUTH_TOKEN

description: |
description: >2
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change wanted? what s >2 doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a convention that Jeff Banks added to the equivalent Docs API tests in the main nosqlbench repo. I think it means that what follows is structured data with 2 space indentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a YAML feature, where there's a myriad of ways to specify how text blocks are included (String values, not YAML markup -- often embedded JSON, XML, CSV, shell scripts)
Which can be convenient but also confusing until you learn all of them when someone uses them (at least I find them confusing).


# spread into different spaces to use multiple connections
space: HashRange(1,<<connections:20>>); ToString();

# http request id
request_id: ToHashedUUID(); ToString();
token: Discard(); Token('<<auth_token:>>','<<uri:http://localhost:8081/v1/auth>>', '<<uid:cassandra>>', '<<pswd:cassandra>>');
Copy link
Contributor

Choose a reason for hiding this comment

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

will this do an auth request on each cycle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, NB caches the token

Copy link
Contributor

Choose a reason for hiding this comment

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

Good. but can you add a small comment here, explaining what the parameters are.. I guess first parameter is token given from the command line and if given, don't call the auth api? In addition it's good to know that the result of the token is saved in the {{token}} var.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comment

@jeffreyscarpenter
Copy link
Contributor Author

The token is added only in the nosqlbench/http-jsonapi-crud-basic.yaml, other files don't have this change.. Will this be updated?

Good catch, thanks. I have pushed a fix so you can re-review.

Copy link
Contributor

@ivansenic ivansenic left a comment

Choose a reason for hiding this comment

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

Looks good, have a look on the remaining comments before merging..

@@ -21,13 +21,14 @@ bindings:
# single host: jsonapi_host=host1
# multiple hosts: jsonapi_host=host1,host2,host3
# multiple weighted hosts: jsonapi_host=host1:3,host2:7
weighted_hosts: WeightedStrings('<<jsonapi_host:jsonapi>>')
weighted_hosts: WeightedStrings('<<jsonapi_host:stargate>>')
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still like why don't we have localhost as default? In addition, this change is also only available in this file, not the others.

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'm going to revert this change for consistency


# spread into different spaces to use multiple connections
space: HashRange(1,<<connections:20>>); ToString();

# http request id
request_id: ToHashedUUID(); ToString();
token: Discard(); Token('<<auth_token:>>','<<uri:http://localhost:8081/v1/auth>>', '<<uid:cassandra>>', '<<pswd:cassandra>>');
Copy link
Contributor

Choose a reason for hiding this comment

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

Good. but can you add a small comment here, explaining what the parameters are.. I guess first parameter is token given from the command line and if given, don't call the auth api? In addition it's good to know that the result of the token is saved in the {{token}} var.

@ivansenic ivansenic mentioned this pull request Apr 24, 2023
4 tasks
@jeffreyscarpenter
Copy link
Contributor Author

FYI @maheshrajamani - I made a couple additional small improvements to address feedback from @ivansenic re-review.

@jeffreyscarpenter jeffreyscarpenter merged commit b6b589a into main Apr 24, 2023
@jeffreyscarpenter jeffreyscarpenter deleted the jeff/nb5-update branch April 24, 2023 17:07
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.

3 participants