Add tags support to gateway backends - #947
Conversation
|
This pull request has gone a while without any activity. Ask for help on #trino-gateway-dev on Trino slack. |
| JdbcConnectionManager connectionManager = createTestingJdbcConnectionManager(dataStoreConfig()); | ||
| HaGatewayManager haGatewayManager = new HaGatewayManager(connectionManager.getJdbi(), new RoutingConfiguration(), new DatabaseCacheConfiguration()); | ||
|
|
||
| var backend = new ProxyBackendConfiguration(); |
There was a problem hiding this comment.
Here's the latest rule:
Use var judiciously
Use var only when it improves readability. Prefer it when the type is obvious from the initializer, such as with new expressions, or when the explicit type is long or heavily generic and adds noise without improving clarity.
Avoid var when the inferred type is unclear, surprising, or important to understanding the code.
https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#use-var-judiciously
| // ----------------------------------------------------------------------- | ||
|
|
||
| @Test | ||
| void tagsToStringReturnsNullForNullInput() |
There was a problem hiding this comment.
prepend method name with test here and other places
| VALUES | ||
| ('trino-1', 'adhoc', 'http://localhost:8081', 'http://localhost:8081', true), | ||
| ('trino-2', 'adhoc', 'http://localhost:8082', 'http://localhost:8082', true); No newline at end of file | ||
| ('trino-1', 'adhoc', 'http://localhost:8081', 'http://localhost:8081', true, 'local,prod'), |
There was a problem hiding this comment.
Is there input validation in place to prevent commas in tag values? what if we have a tag like "version - 480,475"
There was a problem hiding this comment.
Good catch. I have added a validation such that tags cannot have "," in them. Also added a note in the docs to avoid comma and if they need something like "version - 480,475" to use "version - 480","version - 475"
| @Path("/updateBackend") | ||
| public Response updateBackend(ProxyBackendConfiguration backend) | ||
| { | ||
| ProxyBackendConfiguration proxyBackendConfiguration = gatewayBackendManager.updateBackend(backend); |
There was a problem hiding this comment.
If the payload doesn't send the tags, they will be cleared. But this code can inspect the payload and decide to use the sent one vs existing one.
There was a problem hiding this comment.
This is true for every other field. Adding a special update logic for tags might make it inconsistent with other APIs. The UI always sends a full list of tags.
|
This pull request has gone a while without any activity. Ask for help on #trino-gateway-dev on Trino slack. |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
mosabua
added
stale-ignore
|
Can you sqaush & rewrite commit message ? |
@Chaho12 - Done |
|
@vishalya - can you review the changes? |
| checkArgument(backend.getExternalUrl() != null, "Backend external url cannot be null"); | ||
| for (String tag : backend.getTags()) { | ||
| checkArgument(tag != null && !tag.isBlank(), "Backend tag cannot be null or blank"); | ||
| checkArgument(!tag.contains(","), "Backend tag cannot contain ',' character: %s", tag); |
There was a problem hiding this comment.
no need for line 236, as comma is rejected anyway in line 268, as it uses , as storage delimiter.
There was a problem hiding this comment.
The checkArgument is still needed - an un-rejected "a,b" tag would silently split into two tags on read rather than being rejected. This validation was also requested earlier in the review and is covered by tests.
ebyhr
removed
the
stale-ignore
|
This pull request has gone a while without any activity. Ask for help on #trino-gateway-dev on Trino slack. |
|
Build is failing. Please try building locally and fix the failure. |
Done. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughBackend tags are added as a list-based backend configuration field, validated and serialized for database persistence across supported database schemas. Backend responses and frontend cluster management now include tag data for display and editing. Tests cover persistence, updates, clearing, validation, response mapping, and conversion utilities. New documentation describes tag behavior, API examples, validation, and usage scenarios. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/backend-tags.md`:
- Around line 11-13: Update the tag-format description in the backend tags
documentation to state that tags must be nonblank strings, cannot contain
commas, and have surrounding whitespace trimmed. Remove the claim that any
string is accepted and keep the key:value convention as an example rather than a
requirement.
In `@docs/gateway-api.md`:
- Around line 86-87: Update the trino-2 object in the list response example to
include the tags property, using an empty array or representative tag values
consistent with the other objects and the response contract.
In `@webapp/src/components/cluster.tsx`:
- Around line 68-73: Update tagsRender to ensure each rendered Tag has a unique
React key when backend tags contain duplicates, preferably by incorporating the
map index into the key while preserving the existing tag text and styling.
- Around line 222-227: Update the Form.TagInput configuration in the cluster
form to stop treating spaces as tag separators by removing the space entry from
separator. Preserve tags containing spaces as single values while retaining
supported delimiter behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64e5252d-0b2b-455c-85bb-13e935bc54af
📒 Files selected for processing (16)
docs/backend-tags.mddocs/gateway-api.mdgateway-ha/src/main/java/io/trino/gateway/ha/config/ProxyBackendConfiguration.javagateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/GatewayBackend.javagateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/GatewayBackendDao.javagateway-ha/src/main/java/io/trino/gateway/ha/resource/GatewayWebAppResource.javagateway-ha/src/main/java/io/trino/gateway/ha/router/HaGatewayManager.javagateway-ha/src/main/resources/gateway-ha-persistence-mysql.sqlgateway-ha/src/main/resources/gateway-ha-persistence-postgres.sqlgateway-ha/src/main/resources/mysql/V5__add_tags_to_gateway_backend.sqlgateway-ha/src/main/resources/oracle/V5__add_tags_to_gateway_backend.sqlgateway-ha/src/main/resources/postgresql/V5__add_tags_to_gateway_backend.sqlgateway-ha/src/test/java/io/trino/gateway/ha/router/TestHaGatewayManager.javamkdocs.ymlwebapp/src/components/cluster.tsxwebapp/src/types/cluster.d.ts
5 participants
Description
The backend stored in Gateway today has only limited fields (external url, proxy-to, name, active/not-active).
But to make some sophisticated routing decision, having tags on them would help.
For instance the version of the cluster, environment (prod/test), team name etc.
This PR does the following -
tags in INSERT/UPDATE queries
List, with tagsToString/tagsFromString helpers
TagInput in the create/edit modal with space-or-enter to add tags
and × to remove them; editTagsRef ensures tags survive any
Form.TagInput closure/submission quirks
integration tests covering create, update, clear, and list
Additional context and related issues
Tags in UI -
Update box -
Tags updated -
Release notes
Add tags to gateway backends
( ) This is not user-visible or is docs only, and no release notes are required.
( X ) Release notes are required, with the following suggested text:
* Fix some things.