stefan-egli commented on code in PR #635: URL: https://github.com/apache/jackrabbit-oak/pull/635#discussion_r929134054
########## oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/ThrottlingMetrics.java: ########## @@ -0,0 +1,51 @@ +/* + * 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.jackrabbit.oak.plugins.document; + +/** + * Interface to expose throttling metrics. + * + * Concrete implementations of this interface are required to provide implementations + * of this class expose their throttling metrics + */ +public interface ThrottlingMetrics { + + int threshold(); + + double currValue(); + + long throttlingTime(); + + + ThrottlingMetrics DEFAULT_THROTTLING_METRIC = new ThrottlingMetrics() { + + @Override + public int threshold() { + return Integer.MAX_VALUE; + } + + @Override + public double currValue() { + return 0; + } + + @Override + public long throttlingTime() { + return 0; + } + }; Review Comment: Based on the `Throttler` interface sketched above, this could become sth like ```suggestion Throttling NO_THROTTLING = new Throttling() { @Override public long throttlingTime() { return 0; } }; ``` ########## oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentStoreThrottling.java: ########## @@ -0,0 +1,31 @@ +/* + * 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.jackrabbit.oak.plugins.document; + +/** + * Marker interface to check whether underlying document store needs to throttle the incoming requests or not + * + * Concrete implementation needs to provide the logic for implementing the throttling based on their internal usages/statistics + */ +public interface DocumentStoreThrottling { Review Comment: I agree with what @mreutegg wrote : it's not a marker. Plus it's not used anywhere except the implementation. Maybe removing this is more straight forward and doesn't prevent it from being re-added in the future if there's a use case then. ########## oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/ThrottlingMetrics.java: ########## @@ -0,0 +1,51 @@ +/* + * 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.jackrabbit.oak.plugins.document; + +/** + * Interface to expose throttling metrics. + * + * Concrete implementations of this interface are required to provide implementations + * of this class expose their throttling metrics + */ +public interface ThrottlingMetrics { Review Comment: I was wondering if this class could be split along the different concerns it seems to currently combine: config (threshold/throttlingTime) and metric (currValue). What came to mind as an alternative would be something like ``` public interface Throttler { long throttlingTime(); } ``` that could replace the `ThrottlingMetrics`. then you could move the actual algorithm that calculates the throttling time into eg a `ExponentialThrottler` that works based on an AtomicLong (for value updates) and thresholds (these 2 would move here from the current `ThrottlingMetrics`) - and similarly we could come up with different such algorithms for Throttler implementations (which could mean perhaps an interface as its input...) and `MongoThrottlingMetrics` might either not be needed anymore or become the factory that creates an `ExponentialThrottler` with the correct inputs, taken from oplog metric etc. This way essentially the `metrics` part is hidden from the throttling logic and become a factory/implementation detail. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
