[core] Compatibile function for file descriptor #50170
Conversation
dentiny
force-pushed
the
hjiang/compat-for-fd
branch
3 times, most recently
from
f1e4cf0 to
50fc805
Compare
Signed-off-by: dentiny <dentinyhao@gmail.com>
dentiny
force-pushed
the
hjiang/compat-for-fd
branch
from
50fc805 to
7f26146
Compare
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
src/ray/util/compat.cc
Outdated
| } | ||
| return Status::OK(); | ||
| } | ||
| void Flush(MEMFD_TYPE_NON_UNIQUE fd) { |
There was a problem hiding this comment.
No fsyncgate is not recoverable and should exit abnormally directly
There was a problem hiding this comment.
I think as a util, caller should decide whether it wants to exit abnormally or do something else. Otherwise why fsyncgate doesn't exit abnormally inside itself.
There was a problem hiding this comment.
Otherwise why fsyncgate doesn't exit abnormally inside itself
What do you mean? It's a behavior, not a function call
There was a problem hiding this comment.
Are you saying why fsync doesn't exit itself? It's because fsync behavior differs varies among filesystems, for example, ext-4 is known to clear dirty bit after a failed sync behavior, not sure about other fs.
Interface-wise, POSIX doesn't care about fs type.
There was a problem hiding this comment.
I believe postgres panics for all errors: https://github.com/postgres/postgres/blob/43a15eb9400dba2b0b97be72d1a3745a6a6f7136/src/backend/access/transam/xlog.c#L8734-L8745
There was a problem hiding this comment.
The only err code gets retries is signal interrupt:
https://github.com/postgres/postgres/blob/43a15eb9400dba2b0b97be72d1a3745a6a6f7136/src/backend/storage/file/fd.c#L475-L493
There was a problem hiding this comment.
If we really need to handle it via mimic the postgres and tigerbeetles' behavior, we will differentiate (1) windows vs unix; (2) error codes, I would like to keep the code simple, so just panic here.
There was a problem hiding this comment.
I think they assumed fd is always valid. But we are writing a utility, I don't think we should assume that.
There was a problem hiding this comment.
We can also declare the util function is used inside of ray (the filesystem function is not exposed externally);
anyway I updated the return value, PTAL
|
The failed unit test has nothing to do with my change, since it's not used in production even. |
Signed-off-by: dentiny <dentinyhao@gmail.com>
src/ray/util/tests/compat_test.cc
Outdated
| const std::string test_file_path = absl::StrFormat("%s.out", GenerateUUIDV4()); | ||
|
|
||
| HANDLE handle = CreateFile(test_file_path.data(), // File path | ||
| GENERIC_WRITE, // Open for writing | ||
| 0, // No sharing | ||
| NULL, // Default security | ||
| CREATE_ALWAYS, // Create new file (overwrite if exists) | ||
| FILE_ATTRIBUTE_NORMAL, // Normal file attributes | ||
| NULL // No template | ||
| ); | ||
| ASSERT_NE(handle, INVALID_HANDLE_VALUE); | ||
| absl::Cleanup cleanup_test_file = [&test_file_path]() { | ||
| EXPECT_TRUE(std::filesystem::remove(test_file_path)); | ||
| }; |
There was a problem hiding this comment.
We can use the same ScopedTemporaryDirectory?
Actually with this PR, we don't need #if defined(__APPLE__) || defined(__linux__) for the test right?
There was a problem hiding this comment.
I tried for a few times and didn't get it worked, so fall back to the old way
There was a problem hiding this comment.
I think ifdef is needed anyway to get a fd / handle cross-platform
There was a problem hiding this comment.
OK I updated to stdout, which we already have a util function
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
|
I merged #50173, could you update this PR to use the functions defined here and resolve the TODOs in that PR. |
I will have a followup PR to cleanup all cross-platform syscalls after all related PRs merged. |
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
This PR writes multiple compatible functions for file descriptors, which have been used elsewhere.