Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16318 )
Change subject: IMPALA-9867: Add Support for Spilling to S3: Milestone 1 ...................................................................... Patch Set 24: (23 comments) Pushing out review comments for the non-tmp-file-mgr files. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.h File be/src/runtime/io/disk-file.h: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.h@113 PS24, Line 113: DCHECK(status_lock.owns_lock()); nit: can you check here and and GetFileStatusLocked() that status_lock.mutex() is the right lock (i.e. compare the pointers). http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.h@167 PS24, Line 167: SpinLock status_lock_; What about the lock order between 'status_lock_' and 'lock_'. It looks like 'lock_' must be acquired first. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.h@175 PS24, Line 175: boost::shared_mutex lock_; Maybe call this deletion_lock_ or physical_file_lock_ to make it clear that's what it's protecting. Usually I assume something called 'lock_' is protecting internal state of an object. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.h@185 PS24, Line 185: /// Specify if the file's space is reserved. Can you add another sentence or two explaining what reserved means? http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.h@186 PS24, Line 186: AtomicBool space_reserved_{false}; What's the concurrency protocol around this value? It's pretty hard to use atomics correctly so I'd want to clearly document how it's meant to be used. One thing is that it's not immediately clear to me how you would avoid races between writers, i.e. how do you prevent two different threads marking it as reserved at the same time? If there's guaranteed to be only a single writer at a time for other reasons, important to document that here. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.cc File be/src/runtime/io/disk-file.cc: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.cc@37 PS24, Line 37: Not nit: No http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.cc@39 PS24, Line 39: if (!is_deleted(status_l)) { Restructure this conditional to do an early exit, it'll reduce nesting and be more readable. I.e. if (is_deleted(status_l)) return DISK_FILE_DELETE_FAILED_INCORRECT_STATUS; http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.cc@40 PS24, Line 40: status = FileSystemUtil::RemovePaths({path_}); I think we can wrap RemovePaths() here with RETURN_IF_ERROR() here and remove the conditional on l41. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.cc@55 PS24, Line 55: file_writer_.reset(new LocalFileWriter(io_mgr, path.c_str())); nit: you can initialize file_writer_ in the initializer list above instead of using Reset(). http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-file.cc@56 PS24, Line 56: space_reserved_.Store(true); nit: initialize this in the initializer list. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-io-mgr.h File be/src/runtime/io/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-io-mgr.h@81 PS24, Line 81: funtion nit: function http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-io-mgr.cc@109 PS24, Line 109: // The maximum number of HDFS I/O threads for doing file operations, such as file Side note that you don't need to fix here. We have a bit of a flaw in the design here where we keep adding more and more background threads every time we add a new filesystem or work type. We should be lazily creating them when they're actually needed. I prototyped a fix here that I haven't had time to move forward - https://gerrit.cloudera.org/#/c/15472/. Might be worth reconsidering. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-io-mgr.cc@268 PS24, Line 268: Status I think this should be a const& http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/disk-io-mgr.cc@850 PS24, Line 850: uint8_t*) nit: use static_cast instead of c-style cast. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/file-writer.h File be/src/runtime/io/file-writer.h: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/file-writer.h@39 PS24, Line 39: FileWriter(DiskIoMgr* io_mgr, const char* file_path, const int64_t file_size, What happens if max_page_size is > file_size? http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/local-file-writer.h File be/src/runtime/io/local-file-writer.h: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/local-file-writer.h@45 PS24, Line 45: /// Internal function for writing the padding data to a file. It is used when the Why do we need padding? Can't we just upload what we have? http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/local-file-writer.cc File be/src/runtime/io/local-file-writer.cc: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/local-file-writer.cc@34 PS24, Line 34: if (file_ == nullptr) { I'm suspicious about accessing file_ without holding lock, it's not obviously threadsafe. I would just acquire lock_ at the top of the function to make it more obviously correct. Also it'd be better to use an early-exit pattern, i.e. if (file_ != nullptr) return Status::OK(); http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/request-context.cc File be/src/runtime/io/request-context.cc: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/request-context.cc@264 PS24, Line 264: // Execute the callback before decrementing the thread count. Otherwise Please factor this out into a helper function instead of duplicating the code. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/request-context.cc@682 PS24, Line 682: tmporary nit: temporary http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/io/request-ranges.h@279 PS24, Line 279: guranttee guarantee http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/query-state.cc@345 PS24, Line 345: query_id(), query_options().scratch_limit, query_options().max_row_size)); The calculation of the maximum isn't quite right, because the maximum page size is max(default_spillable_buffer_size, round_up_to_power_of_two(max_row_size)). It would be good to simplify this though and the need for an upper bound on page size if possible, though. Can we avoid it by making the upload file size a soft limit rather than a hard limit, and uploading the file once its >= the limit? I don't see there being any major problems with writing slightly large files, but maybe I'm missing something. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/tmp-file-mgr-internal.h File be/src/runtime/tmp-file-mgr-internal.h: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/runtime/tmp-file-mgr-internal.h@386 PS24, Line 386: std::vector<TmpFileMgr::WriteDoneCallback>& write_callbacks, bool is_cancelled); Can you pass this by pointer instead of by reference? That's the convention in the codebase and makes it more obvious it's an output parameter. http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/util/hdfs-util.h File be/src/util/hdfs-util.h: http://gerrit.cloudera.org:8080/#/c/16318/24/be/src/util/hdfs-util.h@28 PS24, Line 28: #define FILESYS_PREFIX_HDFS "hdfs://" Define these as proper variables, we prefer not to use preprocessor macros for constants. The syntax is slightly different compared to constants inside C++ classes. E.g. In .h: extern const char* FILESYS_PREFIX_HDFS; In .cc: const char* FILESYS_PREFIX_HDFS = ""; -- To view, visit http://gerrit.cloudera.org:8080/16318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I419b1d5dbbfe35334d9f964c4b65e553579fdc89 Gerrit-Change-Number: 16318 Gerrit-PatchSet: 24 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 06 Jan 2021 00:16:54 +0000 Gerrit-HasComments: Yes
