Get RocksDB Cloud Tests Passing on Windows #120
Conversation
|
I am still reviewig this, but can you pl squash all the 6 commits into a single commit? That keeps the code history much cleaner |
cloud/cloud_env.cc
Outdated
| @@ -59,6 +60,23 @@ void BucketOptions::SetBucketName(const std::string& bucket, | |||
| } | |||
| } | |||
|
|
|||
| void BucketOptions::SetObjectPath(const std::string& object) { | |||
There was a problem hiding this comment.
(I ask because we can likely offload this to filesystem library)
There was a problem hiding this comment.
The object path is the "directory" under which files are stored in the cloud. Before this change, we stored the input name -- which is typically a directory path -- as-is as the object path, but this fails with Windows-style paths.
Since this object path ends up as part of the URL for the bucket, the path must follow URL syntax. So the object path cannot have "" or ":" in it. This method implements the work to "standardize" the path to something that follows URL syntax.
If there is a means to get something from the filesystem library, I am not sure what it is.
There was a problem hiding this comment.
Can we just require object path to be URL-compatible? It's pretty clear this method is used to configure the path in the cloud, why are we feeding it local directory path with C:\\? We should either take what the user gives us verbatim or return an error if the path is not supported. Transparently changing the value here is weird.
There was a problem hiding this comment.
If we do not transform it, then this method must return a Status to say that it is malformed, which is doable. The places that initialize the object path from dbname will break on Windows. We would need to provide another function potentially to do the transformation.
If that is what you prefer, then it can be done and I will make the corresponding changes.
There was a problem hiding this comment.
The places that initialize the object path from dbname will break on Windows.
Where does that happen? Is it only in tests? If it's only in tests we can have a function that transforms the path before calling SetObjectPath
If that is what you prefer, then it can be done and I will make the corresponding changes.
Yeah that plan sounds good.
Replaces direct use of /tmp, to fix Windows issues - Add ScratchDir/File methods to CloudEnvImpl - Fix the ObjectPath to always be set to unix-style directory - Use RocksDB DestroyDir rather than custom implementation - Change the CopyToFromS3 test to append smaller blocks. Windows barfs when appending large blocks to files (RocksDB, not Cloud issue?)
|
@mrambacher Did you run |
This PR fixes build and test issues with RocksDB Cloud on Windows. Building with USE_AWS=1 now completes successfully, and db_cloud_test all pass.
Note that the tests on Windows still have an issue on application exit, most likely related to not shutting down AWS properly prior to application exist.