You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/10/17 05:27:22 UTC
[GitHub] [hadoop] pranavsaxena-microsoft commented on a diff in pull request #5034: Hadoop 18457 temp
pranavsaxena-microsoft commented on code in PR #5034:
URL: https://github.com/apache/hadoop/pull/5034#discussion_r996593303
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingAnalyzer.java:
##########
@@ -20,16 +20,20 @@
import java.util.Timer;
import java.util.TimerTask;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.atomic.AtomicLong;
Review Comment:
AtomicLong should be before AtomicReference.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingInterceptFactory.java:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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.hadoop.fs.azurebfs.services;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.hadoop.fs.azurebfs.AbfsConfiguration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class to get an instance of throttling intercept class per account
Review Comment:
nit: Add '.' for javadocs parsing
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingInterceptFactory.java:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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.hadoop.fs.azurebfs.services;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.hadoop.fs.azurebfs.AbfsConfiguration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class to get an instance of throttling intercept class per account
+ */
+final class AbfsClientThrottlingInterceptFactory {
+
+ private AbfsClientThrottlingInterceptFactory() {
+ }
+
+ private static final Logger LOG = LoggerFactory.getLogger(
+ AbfsClientThrottlingInterceptFactory.class);
+
+ // Map which stores instance of ThrottlingIntercept class per account
+ private static Map<String, AbfsClientThrottlingIntercept> instanceMapping
+ = new ConcurrentHashMap<>();
+
+ /**
+ *
Review Comment:
Is some description to be added?
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/constants/TestConfigurationKeys.java:
##########
@@ -24,6 +24,8 @@
public final class TestConfigurationKeys {
public static final String FS_AZURE_ACCOUNT_NAME = "fs.azure.account.name";
public static final String FS_AZURE_ABFS_ACCOUNT_NAME = "fs.azure.abfs.account.name";
+
Review Comment:
lets remove the extra line.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingIntercept.java:
##########
@@ -46,25 +49,48 @@ public final class AbfsClientThrottlingIntercept {
private AbfsClientThrottlingAnalyzer readThrottler = null;
private AbfsClientThrottlingAnalyzer writeThrottler = null;
private static boolean isAutoThrottlingEnabled = false;
+ private String accountName = "";
+
+ private synchronized void setIsAutoThrottlingEnabled(boolean autoThrottlingEnabled) {
+ isAutoThrottlingEnabled = autoThrottlingEnabled;
+ }
// Hide default constructor
- private AbfsClientThrottlingIntercept() {
- readThrottler = new AbfsClientThrottlingAnalyzer("read");
- writeThrottler = new AbfsClientThrottlingAnalyzer("write");
+ public AbfsClientThrottlingIntercept(String accountName, AbfsConfiguration abfsConfiguration) {
+ setIsAutoThrottlingEnabled(true);
+ this.readThrottler = new AbfsClientThrottlingAnalyzer("read", abfsConfiguration);
+ this.writeThrottler = new AbfsClientThrottlingAnalyzer("write", abfsConfiguration);
+ this.accountName = accountName;
+ LOG.debug("Client-side throttling is enabled for the ABFS file system for the account : {}", accountName);
}
- public static synchronized void initializeSingleton(boolean enableAutoThrottling) {
- if (!enableAutoThrottling) {
- return;
+ // Hide default constructor
+ private AbfsClientThrottlingIntercept(AbfsConfiguration abfsConfiguration) {
+ readThrottler = new AbfsClientThrottlingAnalyzer("read", abfsConfiguration);
+ writeThrottler = new AbfsClientThrottlingAnalyzer("write", abfsConfiguration);
+ }
+
+ public AbfsClientThrottlingAnalyzer getReadThrottler() {
+ return readThrottler;
+ }
+
+ public AbfsClientThrottlingAnalyzer getWriteThrottler() {
Review Comment:
let the accessType be default.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingAnalyzer.java:
##########
@@ -95,6 +106,16 @@ private AbfsClientThrottlingAnalyzer() {
analysisPeriodMs);
}
+ public void resumeTimer() {
+ blobMetrics = new AtomicReference<AbfsOperationMetrics>(
+ new AbfsOperationMetrics(System.currentTimeMillis()));
+ timer = new Timer(String.format("abfs-timer-client-throttling-analyzer-%s", name), true);
Review Comment:
Lets not create a new timer. Reason being, timer creates a TimerThread object which never stops. This will lead to both thread-count and heap-memory to bad state.
Better is that instead of timer.cancel, do following ->
task.cancel();
timer.purge();
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingIntercept.java:
##########
@@ -46,25 +49,48 @@ public final class AbfsClientThrottlingIntercept {
private AbfsClientThrottlingAnalyzer readThrottler = null;
private AbfsClientThrottlingAnalyzer writeThrottler = null;
private static boolean isAutoThrottlingEnabled = false;
+ private String accountName = "";
+
+ private synchronized void setIsAutoThrottlingEnabled(boolean autoThrottlingEnabled) {
+ isAutoThrottlingEnabled = autoThrottlingEnabled;
+ }
// Hide default constructor
- private AbfsClientThrottlingIntercept() {
- readThrottler = new AbfsClientThrottlingAnalyzer("read");
- writeThrottler = new AbfsClientThrottlingAnalyzer("write");
+ public AbfsClientThrottlingIntercept(String accountName, AbfsConfiguration abfsConfiguration) {
+ setIsAutoThrottlingEnabled(true);
Review Comment:
Lets have setIsAutoThrottlingEnabled(abfsConfiguration.isAutoThrottlingEnabled). It will make constructor agnostic to what logic does the calling method has.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingIntercept.java:
##########
@@ -46,25 +49,48 @@ public final class AbfsClientThrottlingIntercept {
private AbfsClientThrottlingAnalyzer readThrottler = null;
private AbfsClientThrottlingAnalyzer writeThrottler = null;
private static boolean isAutoThrottlingEnabled = false;
+ private String accountName = "";
+
+ private synchronized void setIsAutoThrottlingEnabled(boolean autoThrottlingEnabled) {
+ isAutoThrottlingEnabled = autoThrottlingEnabled;
+ }
// Hide default constructor
- private AbfsClientThrottlingIntercept() {
- readThrottler = new AbfsClientThrottlingAnalyzer("read");
- writeThrottler = new AbfsClientThrottlingAnalyzer("write");
+ public AbfsClientThrottlingIntercept(String accountName, AbfsConfiguration abfsConfiguration) {
+ setIsAutoThrottlingEnabled(true);
+ this.readThrottler = new AbfsClientThrottlingAnalyzer("read", abfsConfiguration);
+ this.writeThrottler = new AbfsClientThrottlingAnalyzer("write", abfsConfiguration);
Review Comment:
Lets have common method to set readThrottler and writeThrottler.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingIntercept.java:
##########
@@ -46,25 +49,48 @@ public final class AbfsClientThrottlingIntercept {
private AbfsClientThrottlingAnalyzer readThrottler = null;
private AbfsClientThrottlingAnalyzer writeThrottler = null;
private static boolean isAutoThrottlingEnabled = false;
+ private String accountName = "";
+
+ private synchronized void setIsAutoThrottlingEnabled(boolean autoThrottlingEnabled) {
+ isAutoThrottlingEnabled = autoThrottlingEnabled;
+ }
// Hide default constructor
- private AbfsClientThrottlingIntercept() {
- readThrottler = new AbfsClientThrottlingAnalyzer("read");
- writeThrottler = new AbfsClientThrottlingAnalyzer("write");
+ public AbfsClientThrottlingIntercept(String accountName, AbfsConfiguration abfsConfiguration) {
+ setIsAutoThrottlingEnabled(true);
+ this.readThrottler = new AbfsClientThrottlingAnalyzer("read", abfsConfiguration);
+ this.writeThrottler = new AbfsClientThrottlingAnalyzer("write", abfsConfiguration);
+ this.accountName = accountName;
+ LOG.debug("Client-side throttling is enabled for the ABFS file system for the account : {}", accountName);
}
- public static synchronized void initializeSingleton(boolean enableAutoThrottling) {
- if (!enableAutoThrottling) {
- return;
+ // Hide default constructor
+ private AbfsClientThrottlingIntercept(AbfsConfiguration abfsConfiguration) {
+ readThrottler = new AbfsClientThrottlingAnalyzer("read", abfsConfiguration);
+ writeThrottler = new AbfsClientThrottlingAnalyzer("write", abfsConfiguration);
+ }
+
+ public AbfsClientThrottlingAnalyzer getReadThrottler() {
Review Comment:
let the accessType be default.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingInterceptFactory.java:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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.hadoop.fs.azurebfs.services;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.hadoop.fs.azurebfs.AbfsConfiguration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class to get an instance of throttling intercept class per account
+ */
+final class AbfsClientThrottlingInterceptFactory {
+
+ private AbfsClientThrottlingInterceptFactory() {
+ }
+
+ private static final Logger LOG = LoggerFactory.getLogger(
+ AbfsClientThrottlingInterceptFactory.class);
+
+ // Map which stores instance of ThrottlingIntercept class per account
+ private static Map<String, AbfsClientThrottlingIntercept> instanceMapping
+ = new ConcurrentHashMap<>();
+
+ /**
+ *
+ * @param accountName The account for which we need instance of throttling intercept
+ @param abfsConfiguration The object of abfsconfiguration class
+ * @return Instance of AbfsClientThrottlingIntercept class
+ */
+ static synchronized AbfsClientThrottlingIntercept getInstance(String accountName,
+ AbfsConfiguration abfsConfiguration) {
+ AbfsClientThrottlingIntercept instance;
+ // If singleton is enabled use a static instance of the intercept class for all accounts
+ if (!abfsConfiguration.isAccountThrottlingEnabled()) {
+ instance = AbfsClientThrottlingIntercept.initializeSingleton(abfsConfiguration);
Review Comment:
Lets have the abfsConfiguration.isAutoThrottlingEnabled() check here. agree that its in initializeSingleton method. Having check here instead of the method will be more readable. Reason being, anyone reading current code, might think that we expect an instance here. Also, initializeSingleton should only create a singleton object and not have any checks (agree that the check was there before as well). What you feel?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientThrottlingIntercept.java:
##########
@@ -46,25 +49,48 @@ public final class AbfsClientThrottlingIntercept {
private AbfsClientThrottlingAnalyzer readThrottler = null;
private AbfsClientThrottlingAnalyzer writeThrottler = null;
private static boolean isAutoThrottlingEnabled = false;
+ private String accountName = "";
+
+ private synchronized void setIsAutoThrottlingEnabled(boolean autoThrottlingEnabled) {
+ isAutoThrottlingEnabled = autoThrottlingEnabled;
+ }
// Hide default constructor
- private AbfsClientThrottlingIntercept() {
- readThrottler = new AbfsClientThrottlingAnalyzer("read");
- writeThrottler = new AbfsClientThrottlingAnalyzer("write");
+ public AbfsClientThrottlingIntercept(String accountName, AbfsConfiguration abfsConfiguration) {
+ setIsAutoThrottlingEnabled(true);
+ this.readThrottler = new AbfsClientThrottlingAnalyzer("read", abfsConfiguration);
+ this.writeThrottler = new AbfsClientThrottlingAnalyzer("write", abfsConfiguration);
Review Comment:
Lets add account in the name. Reason being all threads will have same name.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org