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

[COST-1847] unleash client for trino enablement #3163

Merged
merged 16 commits into from
Oct 4, 2021
Merged

Conversation

maskarb
Copy link
Member

@maskarb maskarb commented Sep 29, 2021

This PR adds the unleash client and allows us to utilize Unleash to enable trino processing for account/schema numbers. All the other mechanisms in place to enable trino processing will still work, so any ENABLE_TRINO_SOURCES, ENABLE_TRINO_SOURCE_TYPE, or ENABLE_TRINO_ACCOUNTS that are already defined will function the same way.

Adding the unleash client here allows us to add new accounts without the hassle of updating the deploy.yaml in app-interface.

Testing:
(this is easier to see without trino running)

  1. make remove-db
  2. make docker-up-min
  3. make create-test-customer
  4. make load-test-customer
  5. See the everything succeeds
  6. Delete all sources and cost models from the db (yea, this is a pain)
  7. Log into unleash (http://localhost:4242/) and add acct10001 to the schema-name list in the schema-strategy for the cost-trino-processor feature flag
  8. recreate the test customers and load in data
  9. watch everything fail to ingest (because all data is going thru trino and trino is not running)

Smokes:
Manually ran the 2 failures:

$ iqe tests plugin cost_management -k test_api_ocp_source_crud_upload_service  -m cost_smoke
1 passed, 6120 deselected, 5 warnings in 224.55s (0:03:44)
$ iqe tests plugin cost_management -k test_api_ocp_sources_shared_rates_recalculation -m cost_smoke
1 passed, 6120 deselected, 5 warnings in 31.30s

@maskarb maskarb added the pr-check-build pr_check will build the image label Sep 29, 2021
@maskarb maskarb self-assigned this Sep 29, 2021
@maskarb
Copy link
Member Author

maskarb commented Sep 29, 2021

/retest

@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #3163 (63e2617) into main (670e19c) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3163     +/-   ##
=======================================
- Coverage   94.0%   94.0%   -0.0%     
=======================================
  Files        308     308             
  Lines      26172   26170      -2     
  Branches    2975    2974      -1     
=======================================
- Hits       24596   24592      -4     
- Misses       997    1000      +3     
+ Partials     579     578      -1     

@maskarb maskarb force-pushed the enable-trino-unleash branch from 84a26f8 to 63c30aa Compare September 29, 2021 18:09
@maskarb maskarb removed the pr-check-build pr_check will build the image label Sep 29, 2021
@maskarb maskarb changed the title initial unleash client for trino enablement [COST-1847] unleash client for trino enablement Sep 29, 2021
@maskarb
Copy link
Member Author

maskarb commented Sep 29, 2021

/retest

@maskarb maskarb marked this pull request as ready for review September 29, 2021 20:50
@maskarb maskarb requested a review from a team September 29, 2021 20:50
@maskarb maskarb added smoke-tests pr_check will build the image and run minimal required smokes and removed trino-smoke-tests labels Sep 29, 2021
@maskarb
Copy link
Member Author

maskarb commented Sep 29, 2021

/retest

@maskarb maskarb removed the smoke-tests pr_check will build the image and run minimal required smokes label Sep 30, 2021
@maskarb maskarb added the smoke-tests pr_check will build the image and run minimal required smokes label Sep 30, 2021
@maskarb
Copy link
Member Author

maskarb commented Sep 30, 2021

/retest

@maskarb maskarb removed the smoke-tests pr_check will build the image and run minimal required smokes label Sep 30, 2021
adberglund
adberglund previously approved these changes Oct 1, 2021
Copy link
Contributor

@adberglund adberglund left a comment

Choose a reason for hiding this comment

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

Approved with a suggestion on formatting the JSON file

@@ -0,0 +1 @@
{"version":1,"features":[{"name":"cost-trino-processor","description":"Toggle to enable trino processing","type":"permission","project":"default","enabled":true,"stale":false,"strategies":[{"name":"schema-strategy","parameters":{"schema-name":""},"constraints":[]}],"variants":[],"createdAt":"2021-09-14T21:22:00.756Z"}],"strategies":[{"name":"schema-strategy","description":"Enablement based on account/schema number","parameters":[{"name":"schema-name","type":"list","description":"values must begin with `acct`","required":false}]}]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{"version":1,"features":[{"name":"cost-trino-processor","description":"Toggle to enable trino processing","type":"permission","project":"default","enabled":true,"stale":false,"strategies":[{"name":"schema-strategy","parameters":{"schema-name":""},"constraints":[]}],"variants":[],"createdAt":"2021-09-14T21:22:00.756Z"}],"strategies":[{"name":"schema-strategy","description":"Enablement based on account/schema number","parameters":[{"name":"schema-name","type":"list","description":"values must begin with `acct`","required":false}]}]}
{
"version": 1,
"features": [
{
"name": "cost-trino-processor",
"description": "Toggle to enable trino processing",
"type": "permission",
"project": "default",
"enabled": true,
"stale": false,
"strategies": [
{
"name": "schema-strategy",
"parameters": {
"schema-name": ""
},
"constraints": []
}
],
"variants": [],
"createdAt": "2021-09-14T21:22:00.756Z"
}
],
"strategies": [
{
"name": "schema-strategy",
"description": "Enablement based on account/schema number",
"parameters": [
{
"name": "schema-name",
"type": "list",
"description": "values must begin with `acct`",
"required": false
}
]
}
]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The main reason why I didn't format this file is that we won't modify it directly. We should be exporting the flags from the unleash DB using make unleash-export. But! I updated that make command to format the file on export, so it should be readable now.

@maskarb maskarb added the smoke-tests pr_check will build the image and run minimal required smokes label Oct 4, 2021
@maskarb
Copy link
Member Author

maskarb commented Oct 4, 2021

/retest

@maskarb maskarb removed the smoke-tests pr_check will build the image and run minimal required smokes label Oct 4, 2021
@maskarb maskarb added the smoke-tests pr_check will build the image and run minimal required smokes label Oct 4, 2021
@maskarb maskarb merged commit 1240bb9 into main Oct 4, 2021
@maskarb maskarb deleted the enable-trino-unleash branch October 4, 2021 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smoke-tests pr_check will build the image and run minimal required smokes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants