cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Ellis (JIRA)" <j...@apache.org>
Subject [jira] Commented: (CASSANDRA-279) finish snapshot support
Date Fri, 17 Jul 2009 02:52:14 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12732317#action_12732317
] 

Jonathan Ellis commented on CASSANDRA-279:
------------------------------------------

thanks for the patch!

i like the optional user-provided tag; good idea there.  I also didn't know about ProcessBuilder
-- learn something every day. :)

some minor changes.  sorry for the long-ish list, most are really quick ones:

- use fooPath for strings that represent fs paths.  so you'd have String snapshotPath and
File snapshotDir.  less confusing than Dir/Directory.

- you aren't modifying the sstables_ map so you only need to take a read lock, even though
you're writing to disk

- prefer File.separator to System.getProperty("file.separator").  prefer new File(parent,
child) to parent + separator + child when a File is needed.

- throw new IOException("Snapshot directory must be set."); -- let's just force this to be
set at config time rather than throw an error during the snapshot process that we can't propagate
up to the user easily.  if we also mkdir it at config time that is one less check we need
to do, too.

- snapshotDirectory, snapshotDir, currentSnapshotDir, snapshotTagDirectory -- can we cut down
on the local vars here at all?  would it help to point out FileUtils.createDirectory which
can create multiple subdirs at once, as necessary?

+       if (clientSuppliedName != null && !clientSuppliedName.equals("")) {^M
+           currentSnapshotDir = snapshotDirectory + System.getProperty("file.separator")
+ System.currentTimeMillis(
+       }^M
+       else^M
+       {^M
+           currentSnapshotDir = snapshotDirectory + System.getProperty("file.separator")
+ System.currentTimeMillis(
+       }^M

here it's better to build the common part of the var, then use one iff to add the tag if necessary.
 this emphasizes the important part of the If and makes it easier to discern what the conditional
is for.

- normally you can use braces or not in one-liners as you please but for isDebugEnabled, always
leave them off; it's too much noise for a debug statment

- remember to set your ide to indent w/ 4 spaces, not tabs

- windows support?  "On Microsoft Windows, hard links can be created using the mklink /H command
on Windows NT 6.0 and later systems (such as Windows Vista), and in earlier systems (Windows
2000, XP) using fsutil hardlink create."  if it's a big deal we can leave this to someone
who actually needs the feature but it seems that if you have Windows available to test on
it's not hard.

> finish snapshot support
> -----------------------
>
>                 Key: CASSANDRA-279
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-279
>             Project: Cassandra
>          Issue Type: New Feature
>          Components: Core
>            Reporter: Jonathan Ellis
>            Assignee: Sammy Yu
>         Attachments: 0001-Work-for-CASSANDRA-279.patch
>
>
> searching for "snapshot" in *.java shows a bunch of code for supporting snapshots via
hard links.
> (this works b/c SSTables are immutable, once created.)
> this used to be more complete but when we dropped the JDK7 requirement we just removed
the code that we couldn't do in JDK6 and hard link support was one of those.
> So what you would need to do here is:
>  * create a hard link method (using Runtime.exec("ln") on linux / os x I imagine)
>  * add a JMX hook to invoke this on the data files (this is where looking at the old
codebase might help); ColumnFamilyStoreMBean.forceFlush is an example of an "Action" jmx interface.
using jconsole to interact with JMX stuff is explained here: http://wiki.apache.org/cassandra/MemtableThresholds
>  * add something to list the snapshots available via JMX
>  * optionally make this all per-Table instead of per-database

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message