[
https://issues.apache.org/jira/browse/HADOOP-18959?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17785574#comment-17785574
]
ASF GitHub Bot commented on HADOOP-18959:
-----------------------------------------
ahmarsuhail commented on code in PR #6240:
URL: https://github.com/apache/hadoop/pull/6240#discussion_r1391235690
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/BlockManagerParams.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.impl.prefetch;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.LocalDirAllocator;
+import org.apache.hadoop.fs.statistics.DurationTrackerFactory;
+
+/**
+ * This class is used to provide params to {@link BlockManager}.
+ */
[email protected]
+public final class BlockManagerParams {
Review Comment:
nit: could you please rename to `BlockManagerParameters` ?
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/BlockManagerParams.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.impl.prefetch;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.LocalDirAllocator;
+import org.apache.hadoop.fs.statistics.DurationTrackerFactory;
+
+/**
+ * This class is used to provide params to {@link BlockManager}.
+ */
[email protected]
+public final class BlockManagerParams {
+
+ /**
+ * Asynchronous tasks are performed in this pool.
+ */
+ private final ExecutorServiceFuturePool futurePool;
Review Comment:
nit: could you add a new line after each of these, makes for slightly easier
reading.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/BlockManagerParams.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.impl.prefetch;
Review Comment:
could you refactor this class, similar to
[S3ClientFactory.S3ClientCreationParameters](https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ClientFactory.java#L98).
That feels like a cleaner way to get and set these parameters.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/BlockManagerParams.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.impl.prefetch;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.LocalDirAllocator;
+import org.apache.hadoop.fs.statistics.DurationTrackerFactory;
+
+/**
+ * This class is used to provide params to {@link BlockManager}.
+ */
[email protected]
+public final class BlockManagerParams {
+
+ /**
+ * Asynchronous tasks are performed in this pool.
+ */
+ private final ExecutorServiceFuturePool futurePool;
+ /**
+ * Information about each block of the underlying file.
+ */
+ private final BlockData blockData;
+ /**
+ * Size of the in-memory cache in terms of number of blocks.
+ */
+ private final int bufferPoolSize;
+ /**
+ * Statistics for the stream.
+ */
+ private final PrefetchingStatistics prefetchingStatistics;
+ /**
+ * The configuration object.
+ */
+ private final Configuration conf;
+ /**
+ * The local dir allocator instance.
+ */
+ private final LocalDirAllocator localDirAllocator;
+ /**
+ * Max blocks count to be kept in cache at any time.
+ */
+ private final int maxBlocksCount;
+ /**
+ * Tracker with statistics to update.
+ */
+ private final DurationTrackerFactory trackerFactory;
+
+ @SuppressWarnings("checkstyle:parameternumber")
+ private BlockManagerParams(ExecutorServiceFuturePool futurePool, BlockData
blockData,
Review Comment:
we're still ending up with a constructor here. Instead of this, look at
[S3ClientFactory.S3ClientCreationParameters](https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ClientFactory.java#L98)
to see how we get and set parameters there, we should follow the same patter
here.
> Use builder for prefetch CachingBlockManager
> --------------------------------------------
>
> Key: HADOOP-18959
> URL: https://issues.apache.org/jira/browse/HADOOP-18959
> Project: Hadoop Common
> Issue Type: Sub-task
> Reporter: Viraj Jasani
> Assignee: Viraj Jasani
> Priority: Major
> Labels: pull-request-available
>
> Some of the recent changes (HADOOP-18399, HADOOP-18291, HADOOP-18829 etc)
> have added more params for prefetch CachingBlockManager c'tor to process
> read/write block requests. They have added too many params and more are
> likely to be introduced later. We should use builder pattern to pass params.
> This would also help consolidating required prefetch params into one single
> place within S3ACachingInputStream, from scattered locations.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
