steveloughran commented on code in PR #5754: URL: https://github.com/apache/hadoop/pull/5754#discussion_r1247931503
########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/Constants.java: ########## @@ -0,0 +1,52 @@ +/* + * 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 java.util.concurrent.TimeUnit; + +/** + * Constants used by prefetch implementations. + */ +public final class Constants { + + private Constants() { + } + + /** + * Prefetch max blocks count config. Review Comment: add {@value} here and below so IDE popups show the value ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/SingleFilePerBlockCache.java: ########## @@ -103,13 +114,17 @@ private enum LockType { READ, WRITE } + private Entry previous; Review Comment: is it really necessary to implement a doubly linked list? isn't there one already in the system? ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/Constants.java: ########## @@ -0,0 +1,52 @@ +/* + * 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 java.util.concurrent.TimeUnit; + +/** + * Constants used by prefetch implementations. + */ +public final class Constants { Review Comment: can we have a different name please; too confusing ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/Constants.java: ########## @@ -0,0 +1,52 @@ +/* + * 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 java.util.concurrent.TimeUnit; + +/** + * Constants used by prefetch implementations. + */ +public final class Constants { + + private Constants() { + } + + /** + * Prefetch max blocks count config. + */ + public static final String FS_PREFETCH_MAX_BLOCKS_COUNT = "fs.prefetch.max.blocks.count"; Review Comment: I think we should use fs.s3a here for per bucket settings. how about just pass in the count as a parameter, rather than a full Configuration object? ########## hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3APrefetchingInputStream.java: ########## @@ -301,4 +307,116 @@ public void testStatusProbesAfterClosingStream() throws Throwable { } + @Test + public void testSeeksWithLruEviction() throws Throwable { + IOStatistics ioStats; + openFS(); + + ExecutorService executorService = Executors.newFixedThreadPool(5, + new ThreadFactoryBuilder() + .setDaemon(true) + .setNameFormat("testSeeksWithLruEviction-%d") + .build()); + + try (FSDataInputStream in = largeFileFS.open(largeFile)) { + ioStats = in.getIOStatistics(); + + // tests to add multiple blocks in the prefetch cache + // and let LRU eviction take place as more cache entries + // are added with multiple block reads. + + executorService.submit(() -> { + byte[] buffer = new byte[blockSize]; + // Don't read block 0 completely + try { + in.read(buffer, 0, blockSize - S_1K * 10); + } catch (IOException e) { + throw new RuntimeException(e); Review Comment: it should be an UncheckedIOException, but why not just `return true` from each closure so they become Callable<Boolean> rather than Runnable, so can throw exceptions -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
