-
Notifications
You must be signed in to change notification settings - Fork 52
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
Feature Toggle Part II: Plumbing HTS #74
Conversation
cluster/configs/src/main/java/com/linkedin/openhouse/cluster/configs/ClusterProperties.java
Show resolved
Hide resolved
.../java/com/linkedin/openhouse/tables/repository/impl/OpenHouseInternalRepositoryImpl.java.rej
Outdated
Show resolved
Hide resolved
...main/java/com/linkedin/openhouse/tables/repository/impl/OpenHouseInternalRepositoryImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good overall, have feedback on:
- Not sharing ToggleEnum
- Impact of MODE=MYSQL on tables-test-fixtures and non-mysql dbs. There seems to be another way of doing this
- some repositories/ services are not enabled, maybe I missed the description on its plan
services/common/src/main/java/com/linkedin/openhouse/common/api/spec/ToggleStatusEnum.java
Outdated
Show resolved
Hide resolved
...es/src/main/java/com/linkedin/openhouse/housetables/api/spec/model/TableToggleStatusKey.java
Outdated
Show resolved
Hide resolved
...ousetables/src/main/java/com/linkedin/openhouse/housetables/api/spec/model/ToggleStatus.java
Outdated
Show resolved
Hide resolved
...es/src/main/java/com/linkedin/openhouse/housetables/controller/ToggleStatusesController.java
Outdated
Show resolved
Hide resolved
...les/src/main/java/com/linkedin/openhouse/housetables/services/ToggleStatusesServiceImpl.java
Outdated
Show resolved
Hide resolved
services/tables/src/main/java/com/linkedin/openhouse/tables/toggle/model/TableToggleStatus.java
Outdated
Show resolved
Hide resolved
.../main/java/com/linkedin/openhouse/tables/toggle/repository/BaseToggleStatusesRepository.java
Show resolved
Hide resolved
...s-test-fixtures/src/main/java/com/linkedin/openhouse/tablestest/SpringH2TestApplication.java
Show resolved
Hide resolved
There are something really weird going on for H2 if using direct JPA to insert the record. I have spent 5+ hrs on that, the gist of it is it doesn't really honor the constraints declared in DDL and somehow H2 supports oracle's dialect as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments.
spec has changed after this, will you raise a PR for docs as well?
I can update this doc, but the changed spec is within HTS and this doc doesn't publish spec for HTS either. Is that still necessary? |
References I found: JetBrains/Exposed#186 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
This PR contains all changes needed to create a new MySQL table so that HTS can accept inquiries with respect to feature-toggle. As a result of this PR, a new table will be bootstrapped within MySQL when HTS is initialized, as well as one entry inserted into this table for demo purpose.
Changes
For all the boxes checked, please include additional details of the changes made in this pull request.
Introducing a new endpoint
/hts/togglestatuses
in HTS, which essentially answers the question: Given a tri-tuplefeatureId
,databaseId
andtableId
, whether the feature is active for the given table.Note that:
tables-services
.tables-service
is only aware an explicit tuple consisting ofdatabaseId, tableId, featureId
. The rule engine within HTS will need to determine, given the set of rules stored in mysql, whether the inquired entity, here specifically fordatabaseId, tableId
has been activated with thefeatureId
data.sql
and PR-review the change. They don't have to make the server side deployment, but manually inserting row into the MySQL backend is also acceptable.databasePattern
andtablePattern
, tests are assuming that rules written in the repository are having the exact match.Testing Done
For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request.
Verified locally that when HTS is bootstrapped, a new table will show up with a record inserted:
Additional Information
For all the boxes checked, include additional details of the changes made in this pull request.