[
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.
