Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12987 )
Change subject: IMPALA-8341: Data cache for remote reads ...................................................................... Patch Set 5: (32 comments) http://gerrit.cloudera.org:8080/#/c/12987/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12987/4//COMMIT_MSG@61 PS4, Line 61: '--data_cache_dir' specifies the base directory in which each Impalad > Can we add a custom cluster test to sanity check that it works end to end. I have a bit of trouble in getting this to work in the mini-cluster. May be it's easier with the dockerised test. http://gerrit.cloudera.org:8080/#/c/12987/4//COMMIT_MSG@62 PS4, Line 62: creates the caching directory > Do we want to consider enabling this by default for the dockerised tests as Definitely. Will also do so for S3 builds. http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/exec/hdfs-scan-node-base.cc@365 PS4, Line 365: "DataCacheHitCount", TUnit::UNIT); > Do we have tests for these to make sure they show up in the profiles and wo Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache-test.cc File be/src/runtime/io/data-cache-test.cc: http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache-test.cc@138 PS4, Line 138: > Move to the other test constants (TEMP_BUFFER_SIZE etc)? Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache-test.cc@226 PS4, Line 226: > nit: This could now be 4 * FLAGS_data_cache_file_max_size ? Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache-test.cc@349 PS4, Line 349: num_entries = 0 > Can they be to separate tests? Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache-test.cc@410 PS4, Line 410: mp_buf > nit: typo Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.h File be/src/runtime/io/data-cache.h: http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.h@83 PS4, Line 83: eviction from it happen s > That's controlled by --data_cache_write_concurrency, right? Mention here? Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.h@87 PS4, Line 87: /// of 4KB so any data inserted will be rounded up to the nearest multiple of 4KB. > Do we plan to look into picking partitions on faster disks with higher prob Ideally, we want to keep the hotter data in the faster media while keeping the lukewarm data in the slower media. Added a TODO. http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.h@88 PS4, Line 88: /// > Yeah this scenario is a bit concerning for me still since it's conceivable I added a "simple" implementation with rw-lock and lazy cache entry eviction. If it's deemed too complicated, please let me know and I can undo it. Also added a test case for it. http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.h@145 PS4, Line 145: , > nit: formatting Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.h@197 PS4, Line 197: /// - removes any stale backing file in this partition > Should we also delete the files when we close them? There's a distinction i This was needed for data-cache-test.cc as we need to close the files before verifying their sizes. However, it seems that we can hide all those internal details in VerifyFileSizes(), which is renamed to CloseAndVerifyFileSizes(); http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.h@203 PS4, Line 203: > Should we pass const CacheKey& here and convert it in the implementation? Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.h@224 PS4, Line 224: void EvictedEnt > nit: VerifyFileSizes Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc File be/src/runtime/io/data-cache.cc: http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@72 PS4, Line 72: "(Advanced) Enable checksumming for the cached buffer."); > static const char*? This is actually a static class member of DataCache. http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@75 PS4, Line 75: namespace io { > Should this be a class, given it has a c'tor and d'tor? Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@173 PS4, Line 173: ock(lock_.get_lock()); : if (UNLIKELY(!file_)) return fals > I think you can merge these two lines, which also reduces the risk that som Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@187 PS4, Line 187: inline > nit: missing include, but we might generally omit this one. Not sure which one you are referring to ? Isn't it in #include "common/names.h" ? http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@186 PS4, Line 186: // already closed. : inline bool Write(int64_t offset, const uint8_t* buffer, int64_t buffer_len) { : DCHECK_EQ(offset % PAGE_SIZE, 0); : DCHECK_LE(offset, current_offset_); : kudu::shared_lock<rw_spinlock> lock(lock_.get_lock()); : if (UNLIKELY(!file_)) return false; : kudu::Status status = file_->Write(offset, Slice(buffer, buffer_len)); : if (UNLIKELY(!status.ok())) { : LOG(ERROR) << Substitute("Failed to write to $0 at offset $1 for $2 bytes: $3", : path_, offset, PrettyPrinter::PrintBytes(buffer_len), status.ToString()); : return false; : } : return true; : } > This might be a good candidate for a separate method, but I don't feel stro Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@235 PS4, Line 235: ains the w > Wrap in PrintBytes, or just put "bytes" into the message Added bytes in the message. Makes it easier to debug in case things go wrong. http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@306 PS4, Line 306: Status DataCache::Partition::Init() This check is racy. More allocation could have happened since 'insertion_offset' is allocated. http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@316 PS4, Line 316: // Check if there is enough space available at this point in time. > nit: can be on a single line Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@391 PS4, Line 391: > What are the cases where this loop runs more than once? Would a single if-s Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@402 PS4, Line 402: kudu::Cache::Handle* handle, const uint8_t* buffer, int64_t buffer_len) { > Does this need to go to line 387 before the return statement in the file lo Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@459 PS4, Line 459: > Is a capacity of 1 really supported? Should we require something that techn Yes, required at least 1 page. http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/data-cache.cc@482 PS4, Line 482: // the extra memory consumption for holding the temporary buffer though. > Is this a valid thing to happen or should we DCHECK? Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/disk-io-mgr.cc@61 PS4, Line 61: must h > "must have"? Such a configuration is not supported anyways, but if people t Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/hdfs-file-reader.cc File be/src/runtime/io/hdfs-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/runtime/io/hdfs-file-reader.cc@153 PS4, Line 153: RETURN_IF_ERROR(io_mgr->ReopenCachedHdfsFileHandle(hdfs_fs_, > VLOG_FILE? Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/util/filesystem-util-test.cc File be/src/util/filesystem-util-test.cc: http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/util/filesystem-util-test.cc@147 PS4, Line 147: file type > file type entries? Done http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/util/filesystem-util.cc File be/src/util/filesystem-util.cc: http://gerrit.cloudera.org:8080/#/c/12987/4/be/src/util/filesystem-util.cc@339 PS4, Line 339: const off_t hole_size = buffer_size * 2; > Kudu's RWFile encapsulates this. Should we use it instead? Done http://gerrit.cloudera.org:8080/#/c/12987/4/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/12987/4/bin/start-impala-cluster.py@118 PS4, Line 118: 0, > "_size" implies that the default should be something like 0? Done http://gerrit.cloudera.org:8080/#/c/12987/4/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/12987/4/common/thrift/metrics.json@433 PS4, Line 433: "description": "Total number of insertion skipped in remote data cache due to concurrency limit.", > Do we have a test for the new metrics to make sure they work as expected? Done -- To view, visit http://gerrit.cloudera.org:8080/12987 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I734803c1c1787c858dc3ffa0a2c0e33e77b12edc Gerrit-Change-Number: 12987 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Sat, 27 Apr 2019 07:22:43 +0000 Gerrit-HasComments: Yes
