collado-mike commented on code in PR #278:
URL: https://github.com/apache/polaris/pull/278#discussion_r1769323642
##########
polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java:
##########
@@ -252,6 +253,12 @@ public void run(PolarisApplicationConfig configuration,
Environment environment)
.servlets()
.addFilter("tracing", new TracingFilter(openTelemetry))
.addMappingForUrlPatterns(EnumSet.of(DispatcherType.REQUEST), true,
"/*");
+
+ environment
+ .servlets()
+ .addFilter("ratelimiter", new
RateLimiterFilter(configuration.getRateLimiterConfig()))
+ .addMappingForUrlPatterns(EnumSet.of(DispatcherType.REQUEST), true,
"/*");
Review Comment:
We should wrap this in a null check. If the user doesn't configure the
`RateLimiterConfig`, we'll throw a NPE
##########
polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterFactory.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.ratelimiter;
+
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import io.dropwizard.jackson.Discoverable;
+import java.util.concurrent.Future;
+
+/**
+ * Interface for constructing a rate limiter given the rate limiting key and
clock. Notably, rate
+ * limiter construction may be asynchronous. This allows fetching information
related to the key.
+ * For example, implementors may wish to fetch per-account rate limit values,
which require lookup
+ * in an external database.
+ */
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
+public interface RateLimiterFactory extends Discoverable {
+ /**
+ * Constructs a rate limiter asynchronously. Callers may choose to set a
timeout on construction.
+ *
+ * @param key The rate limiting key. Rate limiters may optionally choose to
discriminate their
+ * behavior by the key.
+ * @return a Future with the constructed RateLimiter
+ */
+ Future<RateLimiter> createRateLimiter(String key);
Review Comment:
Why is `key` as String? The javadoc is very vague about what the value is
here, but it is pretty clearly a realm. I'd either make it very explicit, by
passing `RealmContext` or generalize it. Maybe parameterize
`RateLimiterFactory` so it accepts a type of `T` - this may be a `RealmContext`
or something else supplied by the filter.
##########
polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterConfig.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.ratelimiter;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/** Configuration for the rate limiter */
+public class RateLimiterConfig {
+ private RateLimiterFactory rateLimiterFactory;
+
+ /**
+ * Rate limiters can be constructed asynchronously, so this config
determines the construction
+ * timeout before we default to a NoOpRateLimiter.
+ */
+ private long constructionTimeoutMillis;
+
+ /**
+ * Since rate limiter construction is asynchronous and has a timeout,
construction may fail. If
+ * this option is enabled, the request will still be allowed when
construction fails.
+ */
+ private boolean allowRequestOnConstructionTimeout;
+
+ @JsonProperty("factory")
+ public void setRateLimiterFactory(RateLimiterFactory rateLimiterFactory) {
+ this.rateLimiterFactory = rateLimiterFactory;
+ }
+
+ @JsonProperty("factory")
+ public RateLimiterFactory getRateLimiterFactory() {
+ return rateLimiterFactory;
+ }
+
+ @JsonProperty("constructionTimeoutMillis")
+ public void setConstructionTimeoutMillis(long constructionTimeoutMillis) {
Review Comment:
You don't need to define the property annotation for every getter/setter.
It's really only needed if the getter/setter method doesn't match the property
in the JSON. E.g., above your `factory` property doesn't match the
setRateLimiterFactory method, so it's needed there, but not here.
##########
polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/ClockImpl.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.ratelimiter;
+
+/** An implementation of our Clock interface that just uses System.nanoTime()
*/
+public class ClockImpl implements Clock {
Review Comment:
This could just be a method reference to `System::nanoTime`
##########
polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/TokenBucketRateLimiter.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.ratelimiter;
+
+/**
+ * Token bucket implementation of a Polaris RateLimiter. Acquires tokens at a
fixed rate and has a
+ * maximum amount of tokens. Each "acquire" costs 1 token.
+ */
+public class TokenBucketRateLimiter implements RateLimiter {
+ private final double tokensPerNano;
+ private final long maxTokens;
+ private final Clock clock;
+
+ private double tokens;
Review Comment:
why is `tokens` a double?
##########
polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/TokenBucketRateLimiter.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.ratelimiter;
+
+/**
+ * Token bucket implementation of a Polaris RateLimiter. Acquires tokens at a
fixed rate and has a
+ * maximum amount of tokens. Each "acquire" costs 1 token.
+ */
+public class TokenBucketRateLimiter implements RateLimiter {
Review Comment:
This seems like it could be a `Semaphore`
##########
polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterConfig.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.ratelimiter;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/** Configuration for the rate limiter */
+public class RateLimiterConfig {
+ private RateLimiterFactory rateLimiterFactory;
+
+ /**
+ * Rate limiters can be constructed asynchronously, so this config
determines the construction
+ * timeout before we default to a NoOpRateLimiter.
+ */
+ private long constructionTimeoutMillis;
+
+ /**
+ * Since rate limiter construction is asynchronous and has a timeout,
construction may fail. If
+ * this option is enabled, the request will still be allowed when
construction fails.
+ */
+ private boolean allowRequestOnConstructionTimeout;
+
+ @JsonProperty("factory")
+ public void setRateLimiterFactory(RateLimiterFactory rateLimiterFactory) {
+ this.rateLimiterFactory = rateLimiterFactory;
Review Comment:
We should add null checks and/or `@Nullable` annotations
##########
polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/Clock.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.ratelimiter;
+
+/** An interface to specify how to retrieve the current wall clock time */
+public interface Clock {
+ /**
+ * @return the current time in nanoseconds
+ */
+ long nanoTime();
Review Comment:
Why don't we use the `java.time.Clock` interface? Is it because the
`nanoTime` method rather than `millis`? Do we really nanosecond precision? Does
it make a difference if the nanosecond time is not exact? The javadocs for the
SDK say it's not entirely accurate.
--
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: commits-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
