Skip to content

Commit b666b09

Browse files
maswinmaswin
committed
Address review comments
1 parent 83816b3 commit b666b09

6 files changed

Lines changed: 105 additions & 29 deletions

File tree

‎docs/backend-tags.md‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,13 @@ curl -X POST http://localhost:8080/gateway/backend/modify/add \
2828
}'
2929
```
3030

31+
!!! note "Commas are not allowed in tag values"
32+
A tag value cannot contain a `,` character — the gateway will reject the
33+
request with an `IllegalArgumentException`. If you need to express
34+
multiple values for the same key, split them into separate tags rather
35+
than combining them with a comma. For example, instead of
36+
`"ver:440,476"`, use two tags: `"ver:440"` and `"ver:476"`.
37+
3138
Tags are included in all backend list responses:
3239

3340
```json
@@ -72,8 +79,16 @@ enhancements could implement using tags as the metadata source.
7279
### Cluster upgrades and canary deployments
7380

7481
- **Version tags** — Tag backends with `ver:476` to identify which Trino version
75-
a cluster is running. Routing rules can use this to drop deprecated session
76-
properties before forwarding queries to older or newer clusters.
82+
a cluster is running. Routing rules can inspect this tag to make
83+
version-aware decisions, for example:
84+
- *Session property compatibility* — drop or rewrite session properties
85+
that have been deprecated or renamed before forwarding a query to an
86+
older or newer cluster, so clients written against one version can still
87+
run against another.
88+
- *Pinning queries to a known-good version* — temporarily route all
89+
traffic from a specific user, source, or query shape to a cluster
90+
tagged with a particular version while investigating a regression
91+
introduced in a newer release.
7792
- **Canary deployments** — Tag a backend with `route_percent:10` to signal that
7893
only 10% of traffic should be sent to that cluster, enabling gradual rollouts
7994
before promoting a new version to full traffic.

‎gateway-ha/src/main/java/io/trino/gateway/ha/config/ProxyBackendConfiguration.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@
1515

1616
import com.fasterxml.jackson.annotation.JsonProperty;
1717
import com.fasterxml.jackson.annotation.JsonSetter;
18+
import com.google.common.collect.ImmutableList;
1819

19-
import java.util.Collections;
2020
import java.util.List;
2121

2222
public class ProxyBackendConfiguration
@@ -26,7 +26,7 @@ public class ProxyBackendConfiguration
2626
private String externalUrl;
2727
private String name;
2828
private String proxyTo;
29-
private List<String> tags = Collections.emptyList();
29+
private List<String> tags = ImmutableList.of();
3030

3131
@JsonProperty
3232
public String getName()
@@ -100,6 +100,6 @@ public List<String> getTags()
100100
@JsonSetter
101101
public void setTags(List<String> tags)
102102
{
103-
this.tags = tags != null ? tags : Collections.emptyList();
103+
this.tags = tags != null ? tags : ImmutableList.of();
104104
}
105105
}

‎gateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/GatewayBackendDao.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public interface GatewayBackendDao
3333

3434
@SqlUpdate(
3535
"""
36-
INSERT INTO gateway_backend (name, routing_group, backend_url, external_url, active)
36+
INSERT INTO gateway_backend (name, routing_group, backend_url, external_url, active, tags)
3737
VALUES (:name, :routingGroup, :backendUrl, :externalUrl, :active, :tags)
3838
""")
3939
void create(String name, String routingGroup, String backendUrl, String externalUrl, boolean active, String tags);

‎gateway-ha/src/main/java/io/trino/gateway/ha/router/HaGatewayManager.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,10 @@ private static void validateBackendConfiguration(ProxyBackendConfiguration backe
230230
checkArgument(backend.getProxyTo() != null, "Backend proxyTo URL cannot be null");
231231
checkArgument(backend.getRoutingGroup() != null, "Backend routing group cannot be null");
232232
checkArgument(backend.getExternalUrl() != null, "Backend external url cannot be null");
233+
for (String tag : backend.getTags()) {
234+
checkArgument(tag != null && !tag.isBlank(), "Backend tag cannot be null or blank");
235+
checkArgument(!tag.contains(","), "Backend tag cannot contain ',' character: %s", tag);
236+
}
233237
}
234238

235239
public void deleteBackend(String name)

‎gateway-ha/src/test/java/io/trino/gateway/ha/router/TestHaGatewayManager.java‎

Lines changed: 68 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ void testAddBackendWithTags()
157157
JdbcConnectionManager connectionManager = createTestingJdbcConnectionManager(dataStoreConfig());
158158
HaGatewayManager haGatewayManager = new HaGatewayManager(connectionManager.getJdbi(), new RoutingConfiguration(), new DatabaseCacheConfiguration());
159159

160-
var backend = new ProxyBackendConfiguration();
160+
ProxyBackendConfiguration backend = new ProxyBackendConfiguration();
161161
backend.setName("tagged-backend");
162162
backend.setRoutingGroup("adhoc");
163163
backend.setProxyTo("tagged.trino.gateway.io");
@@ -167,7 +167,7 @@ void testAddBackendWithTags()
167167

168168
haGatewayManager.addBackend(backend);
169169

170-
var retrieved = haGatewayManager.getBackendByName("tagged-backend").orElseThrow();
170+
ProxyBackendConfiguration retrieved = haGatewayManager.getBackendByName("tagged-backend").orElseThrow();
171171
assertThat(retrieved.getTags()).containsExactly("prod", "us-east-1");
172172
}
173173

@@ -176,7 +176,7 @@ void testAddBackendWithoutTagsReturnsEmptyList()
176176
{
177177
JdbcConnectionManager connectionManager = createTestingJdbcConnectionManager(dataStoreConfig());
178178
HaGatewayManager haGatewayManager = new HaGatewayManager(connectionManager.getJdbi(), new RoutingConfiguration(), new DatabaseCacheConfiguration());
179-
var backend = new ProxyBackendConfiguration();
179+
ProxyBackendConfiguration backend = new ProxyBackendConfiguration();
180180
backend.setName("untagged-backend");
181181
backend.setRoutingGroup("adhoc");
182182
backend.setProxyTo("untagged.trino.gateway.io");
@@ -186,7 +186,7 @@ void testAddBackendWithoutTagsReturnsEmptyList()
186186

187187
haGatewayManager.addBackend(backend);
188188

189-
var retrieved = haGatewayManager.getBackendByName("untagged-backend").orElseThrow();
189+
ProxyBackendConfiguration retrieved = haGatewayManager.getBackendByName("untagged-backend").orElseThrow();
190190
assertThat(retrieved.getTags()).isEmpty();
191191
}
192192

@@ -195,7 +195,7 @@ void testUpdateBackendTags()
195195
{
196196
JdbcConnectionManager connectionManager = createTestingJdbcConnectionManager(dataStoreConfig());
197197
HaGatewayManager haGatewayManager = new HaGatewayManager(connectionManager.getJdbi(), new RoutingConfiguration(), new DatabaseCacheConfiguration());
198-
var backend = new ProxyBackendConfiguration();
198+
ProxyBackendConfiguration backend = new ProxyBackendConfiguration();
199199
backend.setName("update-tags-backend");
200200
backend.setRoutingGroup("adhoc");
201201
backend.setProxyTo("update-tags.trino.gateway.io");
@@ -207,7 +207,7 @@ void testUpdateBackendTags()
207207
backend.setTags(List.of("prod", "critical"));
208208
haGatewayManager.updateBackend(backend);
209209

210-
var retrieved = haGatewayManager.getBackendByName("update-tags-backend").orElseThrow();
210+
ProxyBackendConfiguration retrieved = haGatewayManager.getBackendByName("update-tags-backend").orElseThrow();
211211
assertThat(retrieved.getTags()).containsExactly("prod", "critical");
212212
}
213213

@@ -216,7 +216,7 @@ void testClearBackendTags()
216216
{
217217
JdbcConnectionManager connectionManager = createTestingJdbcConnectionManager(dataStoreConfig());
218218
HaGatewayManager haGatewayManager = new HaGatewayManager(connectionManager.getJdbi(), new RoutingConfiguration(), new DatabaseCacheConfiguration());
219-
var backend = new ProxyBackendConfiguration();
219+
ProxyBackendConfiguration backend = new ProxyBackendConfiguration();
220220
backend.setName("clear-tags-backend");
221221
backend.setRoutingGroup("adhoc");
222222
backend.setProxyTo("clear-tags.trino.gateway.io");
@@ -228,7 +228,7 @@ void testClearBackendTags()
228228
backend.setTags(List.of());
229229
haGatewayManager.updateBackend(backend);
230230

231-
var retrieved = haGatewayManager.getBackendByName("clear-tags-backend").orElseThrow();
231+
ProxyBackendConfiguration retrieved = haGatewayManager.getBackendByName("clear-tags-backend").orElseThrow();
232232
assertThat(retrieved.getTags()).isEmpty();
233233
}
234234

@@ -237,7 +237,7 @@ void testTagsAreIncludedInGetAllBackends()
237237
{
238238
JdbcConnectionManager connectionManager = createTestingJdbcConnectionManager(dataStoreConfig());
239239
HaGatewayManager haGatewayManager = new HaGatewayManager(connectionManager.getJdbi(), new RoutingConfiguration(), new DatabaseCacheConfiguration());
240-
var backend = new ProxyBackendConfiguration();
240+
ProxyBackendConfiguration backend = new ProxyBackendConfiguration();
241241
backend.setName("all-backends-tags-backend");
242242
backend.setRoutingGroup("adhoc");
243243
backend.setProxyTo("all-backends-tags.trino.gateway.io");
@@ -246,14 +246,71 @@ void testTagsAreIncludedInGetAllBackends()
246246
backend.setTags(List.of("env:prod", "region:us-west-2"));
247247
haGatewayManager.addBackend(backend);
248248

249-
var allBackends = haGatewayManager.getAllBackends();
250-
var match = allBackends.stream()
249+
List<ProxyBackendConfiguration> allBackends = haGatewayManager.getAllBackends();
250+
ProxyBackendConfiguration match = allBackends.stream()
251251
.filter(b -> b.getName().equals("all-backends-tags-backend"))
252252
.findFirst()
253253
.orElseThrow();
254254
assertThat(match.getTags()).containsExactly("env:prod", "region:us-west-2");
255255
}
256256

257+
@Test
258+
void testAddBackendRejectsTagWithComma()
259+
{
260+
JdbcConnectionManager connectionManager = createTestingJdbcConnectionManager(dataStoreConfig());
261+
HaGatewayManager haGatewayManager = new HaGatewayManager(connectionManager.getJdbi(), new RoutingConfiguration(), new DatabaseCacheConfiguration());
262+
ProxyBackendConfiguration backend = new ProxyBackendConfiguration();
263+
backend.setName("comma-tag-backend");
264+
backend.setRoutingGroup("adhoc");
265+
backend.setProxyTo("comma-tag.trino.gateway.io");
266+
backend.setExternalUrl("comma-tag.trino.gateway.io");
267+
backend.setActive(true);
268+
backend.setTags(List.of("version - 480,475"));
269+
270+
assertThatThrownBy(() -> haGatewayManager.addBackend(backend))
271+
.isInstanceOf(IllegalArgumentException.class)
272+
.hasMessageContaining("Backend tag cannot contain ',' character")
273+
.hasMessageContaining("version - 480,475");
274+
}
275+
276+
@Test
277+
void testAddBackendRejectsBlankTag()
278+
{
279+
JdbcConnectionManager connectionManager = createTestingJdbcConnectionManager(dataStoreConfig());
280+
HaGatewayManager haGatewayManager = new HaGatewayManager(connectionManager.getJdbi(), new RoutingConfiguration(), new DatabaseCacheConfiguration());
281+
ProxyBackendConfiguration backend = new ProxyBackendConfiguration();
282+
backend.setName("blank-tag-backend");
283+
backend.setRoutingGroup("adhoc");
284+
backend.setProxyTo("blank-tag.trino.gateway.io");
285+
backend.setExternalUrl("blank-tag.trino.gateway.io");
286+
backend.setActive(true);
287+
backend.setTags(List.of("env:prod", " "));
288+
289+
assertThatThrownBy(() -> haGatewayManager.addBackend(backend))
290+
.isInstanceOf(IllegalArgumentException.class)
291+
.hasMessage("Backend tag cannot be null or blank");
292+
}
293+
294+
@Test
295+
void testUpdateBackendRejectsTagWithComma()
296+
{
297+
JdbcConnectionManager connectionManager = createTestingJdbcConnectionManager(dataStoreConfig());
298+
HaGatewayManager haGatewayManager = new HaGatewayManager(connectionManager.getJdbi(), new RoutingConfiguration(), new DatabaseCacheConfiguration());
299+
ProxyBackendConfiguration backend = new ProxyBackendConfiguration();
300+
backend.setName("update-comma-tag-backend");
301+
backend.setRoutingGroup("adhoc");
302+
backend.setProxyTo("update-comma-tag.trino.gateway.io");
303+
backend.setExternalUrl("update-comma-tag.trino.gateway.io");
304+
backend.setActive(true);
305+
backend.setTags(List.of("env:prod"));
306+
haGatewayManager.addBackend(backend);
307+
308+
backend.setTags(List.of("a,b"));
309+
assertThatThrownBy(() -> haGatewayManager.updateBackend(backend))
310+
.isInstanceOf(IllegalArgumentException.class)
311+
.hasMessageContaining("Backend tag cannot contain ',' character");
312+
}
313+
257314
@Test
258315
void testRemoveTrailingSlashInUrl()
259316
{

‎gateway-ha/src/test/java/io/trino/gateway/ha/router/TestHaGatewayManagerTagHelpers.java‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,25 +28,25 @@ final class TestHaGatewayManagerTagHelpers
2828
// -----------------------------------------------------------------------
2929

3030
@Test
31-
void tagsToStringReturnsNullForNullInput()
31+
void testTagsToStringReturnsNullForNullInput()
3232
{
3333
assertThat(tagsToString(null)).isNull();
3434
}
3535

3636
@Test
37-
void tagsToStringReturnsNullForEmptyList()
37+
void testTagsToStringReturnsNullForEmptyList()
3838
{
3939
assertThat(tagsToString(List.of())).isNull();
4040
}
4141

4242
@Test
43-
void tagsToStringReturnsSingleTag()
43+
void testTagsToStringReturnsSingleTag()
4444
{
4545
assertThat(tagsToString(List.of("prod"))).isEqualTo("prod");
4646
}
4747

4848
@Test
49-
void tagsToStringJoinsMultipleTagsWithComma()
49+
void testTagsToStringJoinsMultipleTagsWithComma()
5050
{
5151
assertThat(tagsToString(List.of("prod", "us-east-1", "critical")))
5252
.isEqualTo("prod,us-east-1,critical");
@@ -57,52 +57,52 @@ void tagsToStringJoinsMultipleTagsWithComma()
5757
// -----------------------------------------------------------------------
5858

5959
@Test
60-
void tagsFromStringReturnsEmptyListForNull()
60+
void testTagsFromStringReturnsEmptyListForNull()
6161
{
6262
assertThat(tagsFromString(null)).isEmpty();
6363
}
6464

6565
@Test
66-
void tagsFromStringReturnsEmptyListForEmptyString()
66+
void testTagsFromStringReturnsEmptyListForEmptyString()
6767
{
6868
assertThat(tagsFromString("")).isEmpty();
6969
}
7070

7171
@Test
72-
void tagsFromStringReturnsEmptyListForBlankString()
72+
void testTagsFromStringReturnsEmptyListForBlankString()
7373
{
7474
assertThat(tagsFromString(" ")).isEmpty();
7575
}
7676

7777
@Test
78-
void tagsFromStringReturnsSingleTag()
78+
void testTagsFromStringReturnsSingleTag()
7979
{
8080
assertThat(tagsFromString("prod")).containsExactly("prod");
8181
}
8282

8383
@Test
84-
void tagsFromStringParsesMultipleTags()
84+
void testTagsFromStringParsesMultipleTags()
8585
{
8686
assertThat(tagsFromString("prod,us-east-1,critical"))
8787
.containsExactly("prod", "us-east-1", "critical");
8888
}
8989

9090
@Test
91-
void tagsFromStringTrimsWhitespaceAroundTags()
91+
void testTagsFromStringTrimsWhitespaceAroundTags()
9292
{
9393
assertThat(tagsFromString("prod , us-east-1 , critical"))
9494
.containsExactly("prod", "us-east-1", "critical");
9595
}
9696

9797
@Test
98-
void tagsFromStringOmitsEmptySegments()
98+
void testTagsFromStringOmitsEmptySegments()
9999
{
100100
assertThat(tagsFromString(",prod,,us-east-1,"))
101101
.containsExactly("prod", "us-east-1");
102102
}
103103

104104
@Test
105-
void tagsToStringAndFromStringAreInverses()
105+
void testTagsToStringAndFromStringAreInverses()
106106
{
107107
var original = List.of("prod", "us-east-1", "critical");
108108
assertThat(tagsFromString(tagsToString(original))).isEqualTo(original);

0 commit comments

Comments
 (0)