CASSANDRA-16926 #6
Conversation
Co-authored-by: Benedict Elliott Smith <[email protected]> Co-authored-by: Aleksey Yeschenko <[email protected]>
|
|
||
| public Map<String, TableSnapshot> listSnapshots() | ||
| { | ||
| //<<<<<<< HEAD |
There was a problem hiding this comment.
nit: a leftover from rebasing I gues
| return file.getParent() + File.pathSeparator() + ".." + File.pathSeparator() + file.getParentFile().getName() + File.pathSeparator() + file.getName(); | ||
| } | ||
|
|
||
| private void testEquivalence(String path) throws IOException |
| @@ -139,12 +139,12 @@ public static long getRolesReadCount() | |||
| return rolesTable.metric.readLatency.latency.getCount(); | |||
| } | |||
|
|
|||
| public static RoleOptions getLoginRoleOprions() | |||
| public static RoleOptions getLoginRoleOptions() | |||
There was a problem hiding this comment.
It is been fixed in apache#1179, JFYI.
| private static final Logger logger = LoggerFactory.getLogger(PathUtils.class); | ||
| private static final NoSpamLogger nospam1m = NoSpamLogger.getLogger(logger, 1, TimeUnit.MINUTES); | ||
|
|
||
| private static Consumer<Path> onDeletion = path -> { |
There was a problem hiding this comment.
Should we somehow take care about thread safety for onDeletion?
There was a problem hiding this comment.
That’s best left to the Consumer implementation
| Runtime.getRuntime().addShutdownHook(new Thread(this)); | ||
| isRegistered = true; | ||
| } | ||
| logger.trace("Scheduling deferred {}deletion of file: {}", recursive ? "recursive " : "", path); |
| /** | ||
| * Wrapper around an InputStream that provides no buffering but can decode varints | ||
| * | ||
| * TODO: probably shouldn't use DataInputStream as a parent |
There was a problem hiding this comment.
can you elaborate a bit in the javadoc?
| * | ||
| * TODO codebase probably should not use tryList, as unexpected exceptions are hidden; | ||
| * probably want to introduce e.g. listIfExists | ||
| * TODO codebase probably should not use Paths.get() to ensure we can override the filesystem |
There was a problem hiding this comment.
Depending on how multi-filesystem is implemented, Paths.get(URI) may still be usable. eg. URI scheme represents different file system provider, URI user info represents tenant or file type (commitlog, or sstable) which maybe stored in different locations.
There was a problem hiding this comment.
Paths.get(URI) is fine, but Paths.get(String) unfortunately isn't, at least not universally. I'll update the TODO to distinguish.
| */ | ||
| public File(String path) | ||
| { | ||
| this(path.isEmpty() ? null : filesystem.getPath(path)); |
There was a problem hiding this comment.
Not required for this patch, just sharing some ideas. Different path may have different file systems, eg. commitlog maybe stored locally but sstable will be stored remotely; Or each keyspace may have its own FS.
There was a problem hiding this comment.
We could definitely support multiple file systems, but to do so across the codebase would require phasing out the use of this String constructor, since the paths supplied to java.io.File are plain paths referring to the default file system. I would definitely support this in a future patch, and hopefully this work has made that more manageable.
5 participants
No description provided.