You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/09/30 13:15:21 UTC
[PR] Extract interface for RequestIdGenerator [polaris]
adutra opened a new pull request, #2720:
URL: https://github.com/apache/polaris/pull/2720
Summary of changes:
1. Extracted an interface from `RequestIdGenerator`.
2. The `generateRequestId` method now returns a `Uni<String>` in case custom implementations need to perform I/O or other blocking calls during request ID generation.
3. Also addressed comments in #2602.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #2720:
URL: https://github.com/apache/polaris/pull/2720#issuecomment-3356472738
> Would you mind elaborating a bit on the motivation behind it?
I'm trying to avoid what happened with the `RealmContextFilter` a while ago: #1345.
IOW, we want to make sure that whatever technique is used to generate a request ID, we won't see Quarkus complaining about it being blocking.
> In what situation would we need a custom request ID generator?
I could see e.g. users storing their request IDs in the database, or using Zookeeper's distributed atomic counter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #2720:
URL: https://github.com/apache/polaris/pull/2720
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2397894164
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/DefaultRequestIdGenerator.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.tracing;
+
+import io.smallrye.mutiny.Uni;
+import jakarta.annotation.Nonnull;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Default implementation of {@link RequestIdGenerator}, striking a balance between randomness and
+ * performance.
+ *
+ * <p>The IDs generated by this generator are of the form: {@code UUID_COUNTER}. The UUID part is
+ * randomly generated at startup, and the counter is incremented for each request.
+ *
+ * <p>In the unlikely event that the counter overflows, a new UUID is generated and the counter is
+ * reset to 1.
+ */
+@ApplicationScoped
+public class DefaultRequestIdGenerator implements RequestIdGenerator {
+
+ record RequestId(UUID uuid, long counter) {
+
+ RequestId() {
+ this(UUID.randomUUID(), 1);
+ }
+
+ @Override
+ @Nonnull
+ public String toString() {
+ return String.format("%s_%019d", uuid(), counter());
Review Comment:
Personally, I think it's fine if the requests are only lexicographically sorted across all requests made per node. IMO request ID sorting doesn't mean much for API clients as they also usually log timestamps.
I'm okay to change it, if you both feel there's something better - but just my two cents 😃
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2398250769
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java:
##########
@@ -18,33 +18,62 @@
*/
package org.apache.polaris.service.tracing;
-import jakarta.annotation.Priority;
-import jakarta.enterprise.context.ApplicationScoped;
+import io.smallrye.common.vertx.ContextLocals;
+import io.smallrye.mutiny.Uni;
import jakarta.inject.Inject;
import jakarta.ws.rs.container.ContainerRequestContext;
-import jakarta.ws.rs.container.ContainerRequestFilter;
-import jakarta.ws.rs.container.PreMatching;
-import jakarta.ws.rs.ext.Provider;
+import jakarta.ws.rs.container.ContainerResponseContext;
+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Response;
+import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.polaris.service.config.FilterPriorities;
import org.apache.polaris.service.logging.LoggingConfiguration;
+import org.jboss.resteasy.reactive.server.ServerRequestFilter;
+import org.jboss.resteasy.reactive.server.ServerResponseFilter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
-@PreMatching
-@ApplicationScoped
-@Priority(FilterPriorities.REQUEST_ID_FILTER)
-@Provider
-public class RequestIdFilter implements ContainerRequestFilter {
+public class RequestIdFilter {
public static final String REQUEST_ID_KEY = "requestId";
+ private static final Logger LOGGER = LoggerFactory.getLogger(RequestIdFilter.class);
+
@Inject LoggingConfiguration loggingConfiguration;
@Inject RequestIdGenerator requestIdGenerator;
- @Override
- public void filter(ContainerRequestContext rc) {
+ @ServerRequestFilter(preMatching = true, priority = FilterPriorities.REQUEST_ID_FILTER)
+ public Uni<Response> assignRequestId(ContainerRequestContext rc) {
var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
- if (requestId == null) {
- requestId = requestIdGenerator.generateRequestId();
+ return (requestId != null
+ ? Uni.createFrom().item(requestId)
+ : requestIdGenerator.generateRequestId(rc))
+ .onItem()
+ .invoke(id -> rc.setProperty(REQUEST_ID_KEY, id))
+ .invoke(id -> ContextLocals.put(REQUEST_ID_KEY, id))
Review Comment:
FYI: #2747 (removed `ContextLocals` and seized the opportunity for some minor cleanup).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2394991044
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/DefaultRequestIdGenerator.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.tracing;
+
+import io.smallrye.mutiny.Uni;
+import jakarta.annotation.Nonnull;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Default implementation of {@link RequestIdGenerator}, striking a balance between randomness and
+ * performance.
+ *
+ * <p>The IDs generated by this generator are of the form: {@code UUID_COUNTER}. The UUID part is
+ * randomly generated at startup, and the counter is incremented for each request.
+ *
+ * <p>In the unlikely event that the counter overflows, a new UUID is generated and the counter is
+ * reset to 1.
+ */
+@ApplicationScoped
+public class DefaultRequestIdGenerator implements RequestIdGenerator {
+
+ record RequestId(UUID uuid, long counter) {
+
+ RequestId() {
+ this(UUID.randomUUID(), 1);
+ }
+
+ @Override
+ @Nonnull
+ public String toString() {
+ return String.format("%s_%019d", uuid(), counter());
Review Comment:
I'm not sure lexicographic sort order is desirable for anything else than making it easier for humans to visualize and compare request IDs – so any of your suggestions is fine 😄
How about we get this in without changing the generation logic, and then, we look into ways of making the logic better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2398903578
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java:
##########
@@ -18,33 +18,62 @@
*/
package org.apache.polaris.service.tracing;
-import jakarta.annotation.Priority;
-import jakarta.enterprise.context.ApplicationScoped;
+import io.smallrye.common.vertx.ContextLocals;
+import io.smallrye.mutiny.Uni;
import jakarta.inject.Inject;
import jakarta.ws.rs.container.ContainerRequestContext;
-import jakarta.ws.rs.container.ContainerRequestFilter;
-import jakarta.ws.rs.container.PreMatching;
-import jakarta.ws.rs.ext.Provider;
+import jakarta.ws.rs.container.ContainerResponseContext;
+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Response;
+import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.polaris.service.config.FilterPriorities;
import org.apache.polaris.service.logging.LoggingConfiguration;
+import org.jboss.resteasy.reactive.server.ServerRequestFilter;
+import org.jboss.resteasy.reactive.server.ServerResponseFilter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
-@PreMatching
-@ApplicationScoped
-@Priority(FilterPriorities.REQUEST_ID_FILTER)
-@Provider
-public class RequestIdFilter implements ContainerRequestFilter {
+public class RequestIdFilter {
public static final String REQUEST_ID_KEY = "requestId";
+ private static final Logger LOGGER = LoggerFactory.getLogger(RequestIdFilter.class);
+
@Inject LoggingConfiguration loggingConfiguration;
@Inject RequestIdGenerator requestIdGenerator;
- @Override
- public void filter(ContainerRequestContext rc) {
+ @ServerRequestFilter(preMatching = true, priority = FilterPriorities.REQUEST_ID_FILTER)
+ public Uni<Response> assignRequestId(ContainerRequestContext rc) {
var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
- if (requestId == null) {
- requestId = requestIdGenerator.generateRequestId();
+ return (requestId != null
+ ? Uni.createFrom().item(requestId)
+ : requestIdGenerator.generateRequestId(rc))
+ .onItem()
+ .invoke(id -> rc.setProperty(REQUEST_ID_KEY, id))
+ .invoke(id -> ContextLocals.put(REQUEST_ID_KEY, id))
Review Comment:
**UPDATE**: I actually remember now 😄
`ContextLocals` _is required_ in this specific place:
https://github.com/apache/polaris/blob/20febdaede19fb7c46e120652fdd1a262c2138e4/runtime/service/src/main/java/org/apache/polaris/service/metrics/RealmIdTagContributor.java#L41
I even remember asking the Quarkus devs for this feature:
https://github.com/quarkusio/quarkus/pull/47887
So, TLDR: we need to keep `ContextLocals` in `RealmContextFilter`. As for `RequestIdFilter` we can remove it, at least for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2394650451
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java:
##########
@@ -18,33 +18,62 @@
*/
package org.apache.polaris.service.tracing;
-import jakarta.annotation.Priority;
-import jakarta.enterprise.context.ApplicationScoped;
+import io.smallrye.common.vertx.ContextLocals;
+import io.smallrye.mutiny.Uni;
import jakarta.inject.Inject;
import jakarta.ws.rs.container.ContainerRequestContext;
-import jakarta.ws.rs.container.ContainerRequestFilter;
-import jakarta.ws.rs.container.PreMatching;
-import jakarta.ws.rs.ext.Provider;
+import jakarta.ws.rs.container.ContainerResponseContext;
+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Response;
+import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.polaris.service.config.FilterPriorities;
import org.apache.polaris.service.logging.LoggingConfiguration;
+import org.jboss.resteasy.reactive.server.ServerRequestFilter;
+import org.jboss.resteasy.reactive.server.ServerResponseFilter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
-@PreMatching
-@ApplicationScoped
-@Priority(FilterPriorities.REQUEST_ID_FILTER)
-@Provider
-public class RequestIdFilter implements ContainerRequestFilter {
+public class RequestIdFilter {
public static final String REQUEST_ID_KEY = "requestId";
+ private static final Logger LOGGER = LoggerFactory.getLogger(RequestIdFilter.class);
+
@Inject LoggingConfiguration loggingConfiguration;
@Inject RequestIdGenerator requestIdGenerator;
- @Override
- public void filter(ContainerRequestContext rc) {
+ @ServerRequestFilter(preMatching = true, priority = FilterPriorities.REQUEST_ID_FILTER)
+ public Uni<Response> assignRequestId(ContainerRequestContext rc) {
var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
- if (requestId == null) {
- requestId = requestIdGenerator.generateRequestId();
+ return (requestId != null
+ ? Uni.createFrom().item(requestId)
+ : requestIdGenerator.generateRequestId(rc))
+ .onItem()
+ .invoke(id -> rc.setProperty(REQUEST_ID_KEY, id))
+ .invoke(id -> ContextLocals.put(REQUEST_ID_KEY, id))
Review Comment:
Same as we do in `RealmContextFilter`: this places the request ID in the current Vertx context for later retrieval by another event loop thread:
https://quarkus.io/guides/duplicated-context#context-local-data
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2394936236
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/DefaultRequestIdGenerator.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.tracing;
+
+import io.smallrye.mutiny.Uni;
+import jakarta.annotation.Nonnull;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Default implementation of {@link RequestIdGenerator}, striking a balance between randomness and
+ * performance.
+ *
+ * <p>The IDs generated by this generator are of the form: {@code UUID_COUNTER}. The UUID part is
+ * randomly generated at startup, and the counter is incremented for each request.
+ *
+ * <p>In the unlikely event that the counter overflows, a new UUID is generated and the counter is
+ * reset to 1.
+ */
+@ApplicationScoped
+public class DefaultRequestIdGenerator implements RequestIdGenerator {
+
+ record RequestId(UUID uuid, long counter) {
+
+ RequestId() {
+ this(UUID.randomUUID(), 1);
+ }
+
+ @Override
+ @Nonnull
+ public String toString() {
+ return String.format("%s_%019d", uuid(), counter());
Review Comment:
nit: if lexicographic ordering is desired (based on previous conversations), it might be worth using something like `<MILLIS_SINCE_EPOCH_PADDED>_<COUNTER_PADDED>_<UUID>` WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2392709683
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java:
##########
@@ -18,33 +18,62 @@
*/
package org.apache.polaris.service.tracing;
-import jakarta.annotation.Priority;
-import jakarta.enterprise.context.ApplicationScoped;
+import io.smallrye.common.vertx.ContextLocals;
+import io.smallrye.mutiny.Uni;
import jakarta.inject.Inject;
import jakarta.ws.rs.container.ContainerRequestContext;
-import jakarta.ws.rs.container.ContainerRequestFilter;
-import jakarta.ws.rs.container.PreMatching;
-import jakarta.ws.rs.ext.Provider;
+import jakarta.ws.rs.container.ContainerResponseContext;
+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Response;
+import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.polaris.service.config.FilterPriorities;
import org.apache.polaris.service.logging.LoggingConfiguration;
+import org.jboss.resteasy.reactive.server.ServerRequestFilter;
+import org.jboss.resteasy.reactive.server.ServerResponseFilter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
-@PreMatching
-@ApplicationScoped
-@Priority(FilterPriorities.REQUEST_ID_FILTER)
-@Provider
-public class RequestIdFilter implements ContainerRequestFilter {
+public class RequestIdFilter {
public static final String REQUEST_ID_KEY = "requestId";
+ private static final Logger LOGGER = LoggerFactory.getLogger(RequestIdFilter.class);
+
@Inject LoggingConfiguration loggingConfiguration;
@Inject RequestIdGenerator requestIdGenerator;
- @Override
- public void filter(ContainerRequestContext rc) {
+ @ServerRequestFilter(preMatching = true, priority = FilterPriorities.REQUEST_ID_FILTER)
+ public Uni<Response> assignRequestId(ContainerRequestContext rc) {
var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
- if (requestId == null) {
- requestId = requestIdGenerator.generateRequestId();
+ return (requestId != null
+ ? Uni.createFrom().item(requestId)
+ : requestIdGenerator.generateRequestId(rc))
+ .onItem()
+ .invoke(id -> rc.setProperty(REQUEST_ID_KEY, id))
+ .invoke(id -> ContextLocals.put(REQUEST_ID_KEY, id))
Review Comment:
What is this for?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2398053623
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java:
##########
@@ -18,33 +18,62 @@
*/
package org.apache.polaris.service.tracing;
-import jakarta.annotation.Priority;
-import jakarta.enterprise.context.ApplicationScoped;
+import io.smallrye.common.vertx.ContextLocals;
+import io.smallrye.mutiny.Uni;
import jakarta.inject.Inject;
import jakarta.ws.rs.container.ContainerRequestContext;
-import jakarta.ws.rs.container.ContainerRequestFilter;
-import jakarta.ws.rs.container.PreMatching;
-import jakarta.ws.rs.ext.Provider;
+import jakarta.ws.rs.container.ContainerResponseContext;
+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Response;
+import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.polaris.service.config.FilterPriorities;
import org.apache.polaris.service.logging.LoggingConfiguration;
+import org.jboss.resteasy.reactive.server.ServerRequestFilter;
+import org.jboss.resteasy.reactive.server.ServerResponseFilter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
-@PreMatching
-@ApplicationScoped
-@Priority(FilterPriorities.REQUEST_ID_FILTER)
-@Provider
-public class RequestIdFilter implements ContainerRequestFilter {
+public class RequestIdFilter {
public static final String REQUEST_ID_KEY = "requestId";
+ private static final Logger LOGGER = LoggerFactory.getLogger(RequestIdFilter.class);
+
@Inject LoggingConfiguration loggingConfiguration;
@Inject RequestIdGenerator requestIdGenerator;
- @Override
- public void filter(ContainerRequestContext rc) {
+ @ServerRequestFilter(preMatching = true, priority = FilterPriorities.REQUEST_ID_FILTER)
+ public Uni<Response> assignRequestId(ContainerRequestContext rc) {
var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
- if (requestId == null) {
- requestId = requestIdGenerator.generateRequestId();
+ return (requestId != null
+ ? Uni.createFrom().item(requestId)
+ : requestIdGenerator.generateRequestId(rc))
+ .onItem()
+ .invoke(id -> rc.setProperty(REQUEST_ID_KEY, id))
+ .invoke(id -> ContextLocals.put(REQUEST_ID_KEY, id))
Review Comment:
> How would you compare both approaches?
Indeed, a request-scoped bean is undoubtedly better.
> So while I see that we are putting the information in RealmContextFilter, what is our (proposed?) use of this?
This was done a while ago, as I thought at that time that we would be leveraging context propagation to "transfer" some information from the initial request scope to, say, the async tasks framework.
It seems though that, since that time, we've been taking the opposite direction: that of _avoiding_ context propagation (due to some issues with bean proxies, etc.)
The situation today is that those calls to `ContextLocals` are probably not necessary anymore. They can be removed.
Are we OK if I remove the call to `ContextLocals` in `RequestIdFilter` in this PR, then remove the other call in `RealmContextFilter` in a different PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #2720:
URL: https://github.com/apache/polaris/pull/2720#issuecomment-3360293659
FYI. One thing that annoys me a bit but I didn't want to change it without prior discussion:
The classes `RequestIdFilter`, `RequestIdGenerator` and `RequestIdResponseFilter` are all in the `org.apache.polaris.service.tracing` package – but the configuration they use is declared in the `org.apache.polaris.service.logging` package and the configuration option is prefixed with `polaris.log`.
I find this setup a little confusing, especially for developers.
I can provide a "fix" for that later if you all agree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2394949212
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/DefaultRequestIdGenerator.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.tracing;
+
+import io.smallrye.mutiny.Uni;
+import jakarta.annotation.Nonnull;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Default implementation of {@link RequestIdGenerator}, striking a balance between randomness and
+ * performance.
+ *
+ * <p>The IDs generated by this generator are of the form: {@code UUID_COUNTER}. The UUID part is
+ * randomly generated at startup, and the counter is incremented for each request.
+ *
+ * <p>In the unlikely event that the counter overflows, a new UUID is generated and the counter is
+ * reset to 1.
+ */
+@ApplicationScoped
+public class DefaultRequestIdGenerator implements RequestIdGenerator {
+
+ record RequestId(UUID uuid, long counter) {
+
+ RequestId() {
+ this(UUID.randomUUID(), 1);
+ }
+
+ @Override
+ @Nonnull
+ public String toString() {
+ return String.format("%s_%019d", uuid(), counter());
Review Comment:
or `<MILLIS_PADDED>_<SMALL_COUNTER>_<UUID>` - `SMALL_COUNTER` meaning something in the range of 10K to avoid calling the system clock too often.
but this might be getting too much into the monotonic clock ares :sweat_smile:
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/DefaultRequestIdGenerator.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.tracing;
+
+import io.smallrye.mutiny.Uni;
+import jakarta.annotation.Nonnull;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Default implementation of {@link RequestIdGenerator}, striking a balance between randomness and
+ * performance.
+ *
+ * <p>The IDs generated by this generator are of the form: {@code UUID_COUNTER}. The UUID part is
+ * randomly generated at startup, and the counter is incremented for each request.
+ *
+ * <p>In the unlikely event that the counter overflows, a new UUID is generated and the counter is
+ * reset to 1.
+ */
+@ApplicationScoped
+public class DefaultRequestIdGenerator implements RequestIdGenerator {
+
+ record RequestId(UUID uuid, long counter) {
+
+ RequestId() {
+ this(UUID.randomUUID(), 1);
+ }
+
+ @Override
+ @Nonnull
+ public String toString() {
+ return String.format("%s_%019d", uuid(), counter());
Review Comment:
or `<MILLIS_PADDED>_<SMALL_COUNTER>_<UUID>` - `SMALL_COUNTER` meaning something in the range of 10K to avoid calling the system clock too often.
but this might be getting too much into the monotonic clock area :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2392769717
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java:
##########
@@ -19,38 +19,21 @@
package org.apache.polaris.service.tracing;
-import com.google.common.annotations.VisibleForTesting;
-import jakarta.enterprise.context.ApplicationScoped;
-import java.util.UUID;
-import java.util.concurrent.atomic.AtomicReference;
+import io.smallrye.mutiny.Uni;
+import jakarta.ws.rs.container.ContainerRequestContext;
-@ApplicationScoped
-public class RequestIdGenerator {
- static final Long COUNTER_SOFT_MAX = Long.MAX_VALUE / 2;
-
- record State(String uuid, long counter) {
-
- State() {
- this(UUID.randomUUID().toString(), 1);
- }
-
- String requestId() {
- return String.format("%s_%019d", uuid, counter);
- }
-
- State increment() {
- return counter >= COUNTER_SOFT_MAX ? new State() : new State(uuid, counter + 1);
- }
- }
-
- final AtomicReference<State> state = new AtomicReference<>(new State());
-
- public String generateRequestId() {
- return state.getAndUpdate(State::increment).requestId();
- }
-
- @VisibleForTesting
- public void setCounter(long counter) {
- state.set(new State(state.get().uuid, counter));
- }
+/**
+ * A generator for request IDs.
+ *
+ * @see RequestIdFilter
+ */
+public interface RequestIdGenerator {
+
+ /**
+ * Generates a new request ID. IDs must be fast to generate and unique. If the generation involves
+ * I/O (which is not recommended), it should be performed asynchronously.
Review Comment:
Can we remove these comments, if I/O involving is not recommended?
```suggestion
* Generates a new request ID. IDs must be fast to generate and unique.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #2720:
URL: https://github.com/apache/polaris/pull/2720#issuecomment-3352119657
cc @adnanhemani
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2397802588
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java:
##########
@@ -18,33 +18,62 @@
*/
package org.apache.polaris.service.tracing;
-import jakarta.annotation.Priority;
-import jakarta.enterprise.context.ApplicationScoped;
+import io.smallrye.common.vertx.ContextLocals;
+import io.smallrye.mutiny.Uni;
import jakarta.inject.Inject;
import jakarta.ws.rs.container.ContainerRequestContext;
-import jakarta.ws.rs.container.ContainerRequestFilter;
-import jakarta.ws.rs.container.PreMatching;
-import jakarta.ws.rs.ext.Provider;
+import jakarta.ws.rs.container.ContainerResponseContext;
+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Response;
+import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.polaris.service.config.FilterPriorities;
import org.apache.polaris.service.logging.LoggingConfiguration;
+import org.jboss.resteasy.reactive.server.ServerRequestFilter;
+import org.jboss.resteasy.reactive.server.ServerResponseFilter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
-@PreMatching
-@ApplicationScoped
-@Priority(FilterPriorities.REQUEST_ID_FILTER)
-@Provider
-public class RequestIdFilter implements ContainerRequestFilter {
+public class RequestIdFilter {
public static final String REQUEST_ID_KEY = "requestId";
+ private static final Logger LOGGER = LoggerFactory.getLogger(RequestIdFilter.class);
+
@Inject LoggingConfiguration loggingConfiguration;
@Inject RequestIdGenerator requestIdGenerator;
- @Override
- public void filter(ContainerRequestContext rc) {
+ @ServerRequestFilter(preMatching = true, priority = FilterPriorities.REQUEST_ID_FILTER)
+ public Uni<Response> assignRequestId(ContainerRequestContext rc) {
var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
- if (requestId == null) {
- requestId = requestIdGenerator.generateRequestId();
+ return (requestId != null
+ ? Uni.createFrom().item(requestId)
+ : requestIdGenerator.generateRequestId(rc))
+ .onItem()
+ .invoke(id -> rc.setProperty(REQUEST_ID_KEY, id))
+ .invoke(id -> ContextLocals.put(REQUEST_ID_KEY, id))
Review Comment:
In a slightly orthogonal direction: I don't see us using the variables we've already stored in ContextLocals anywhere. So while I see that we are putting the information in `RealmContextFilter`, what is our (proposed?) use of this?
Another thought (I'm not sure if this actually answers the question) - if we were to use the request ID later downstream in the application (for example, Event Listeners), would it be better to use `ContextLocals` or `ContainerRequestContext`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2394998688
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/DefaultRequestIdGenerator.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.tracing;
+
+import io.smallrye.mutiny.Uni;
+import jakarta.annotation.Nonnull;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Default implementation of {@link RequestIdGenerator}, striking a balance between randomness and
+ * performance.
+ *
+ * <p>The IDs generated by this generator are of the form: {@code UUID_COUNTER}. The UUID part is
+ * randomly generated at startup, and the counter is incremented for each request.
+ *
+ * <p>In the unlikely event that the counter overflows, a new UUID is generated and the counter is
+ * reset to 1.
+ */
+@ApplicationScoped
+public class DefaultRequestIdGenerator implements RequestIdGenerator {
+
+ record RequestId(UUID uuid, long counter) {
+
+ RequestId() {
+ this(UUID.randomUUID(), 1);
+ }
+
+ @Override
+ @Nonnull
+ public String toString() {
+ return String.format("%s_%019d", uuid(), counter());
Review Comment:
Absolutely :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #2720:
URL: https://github.com/apache/polaris/pull/2720#discussion_r2396004283
##########
runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java:
##########
@@ -18,33 +18,62 @@
*/
package org.apache.polaris.service.tracing;
-import jakarta.annotation.Priority;
-import jakarta.enterprise.context.ApplicationScoped;
+import io.smallrye.common.vertx.ContextLocals;
+import io.smallrye.mutiny.Uni;
import jakarta.inject.Inject;
import jakarta.ws.rs.container.ContainerRequestContext;
-import jakarta.ws.rs.container.ContainerRequestFilter;
-import jakarta.ws.rs.container.PreMatching;
-import jakarta.ws.rs.ext.Provider;
+import jakarta.ws.rs.container.ContainerResponseContext;
+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Response;
+import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.polaris.service.config.FilterPriorities;
import org.apache.polaris.service.logging.LoggingConfiguration;
+import org.jboss.resteasy.reactive.server.ServerRequestFilter;
+import org.jboss.resteasy.reactive.server.ServerResponseFilter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
-@PreMatching
-@ApplicationScoped
-@Priority(FilterPriorities.REQUEST_ID_FILTER)
-@Provider
-public class RequestIdFilter implements ContainerRequestFilter {
+public class RequestIdFilter {
public static final String REQUEST_ID_KEY = "requestId";
+ private static final Logger LOGGER = LoggerFactory.getLogger(RequestIdFilter.class);
+
@Inject LoggingConfiguration loggingConfiguration;
@Inject RequestIdGenerator requestIdGenerator;
- @Override
- public void filter(ContainerRequestContext rc) {
+ @ServerRequestFilter(preMatching = true, priority = FilterPriorities.REQUEST_ID_FILTER)
+ public Uni<Response> assignRequestId(ContainerRequestContext rc) {
var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
- if (requestId == null) {
- requestId = requestIdGenerator.generateRequestId();
+ return (requestId != null
+ ? Uni.createFrom().item(requestId)
+ : requestIdGenerator.generateRequestId(rc))
+ .onItem()
+ .invoke(id -> rc.setProperty(REQUEST_ID_KEY, id))
+ .invoke(id -> ContextLocals.put(REQUEST_ID_KEY, id))
Review Comment:
IIUC, `ContextLocals` seems a global container to hold anything could be used within one request context. Another option is to use a explicit `RequestScoped` bean to store these, like we did in the call context bean. In that case, it's a more typed solution. How would you compare both approaches?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract interface for RequestIdGenerator [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on PR #2720:
URL: https://github.com/apache/polaris/pull/2720#issuecomment-3360064638
> > In what situation would we need a custom request ID generator?
> I could see e.g. users storing their request IDs in the database, or using Zookeeper's distributed atomic counter.
To be clear, I am mostly fine with the changes and asking mainly to learn here - isn't the expected workflow that, in the case users want to store their request IDs in a database prior to the API call, that they would store their request IDs on client-side and pass it into Polaris via the request headers? Can you please explain further in what use case the server-side would actually want to store (and then use) pre-computed request IDs?
I think ZK's distributed atomic counter would be a good use case where making request ID asynchronous would help, although I'm sure most developers nowadays would prefer not to use ZK for a use case like creating request IDs unless they have no other option :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org