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

Marcus Eriksson commented on CASSANDRA-6719:
--------------------------------------------

pushed a commit with the comments addressed 
[here|https://github.com/krummas/cassandra/commits/marcuse/6719]

bq. FSUtils.handleCorruptSSTable/handleFSError are no longer called
this is on purpose and we should probably fix this in 3.0+ as well - I don't 
think we want to trigger the disk failure policy if import fails - instead we 
should abort the import. If someone has configured {{disk_failure_policy: 
die_paranoid}} trying to load a corrupt file would actually stop the node
bq. Row cache invalidation was not previously performed — this is a good thing 
regardless, so maybe skip an option for this one. 
added an option to explicitly skip the row cache invalidation - also added a 
{{--quick}} option which makes it behave more like the old version
bq. If using nodetool refresh with JBOD, the counting keys per boundary work is 
done just to throw it away.
added a check if there is only a single data directory (and row cache is not 
enabled)
bq. Minor/naming nit: consider renaming CFS#loadSSTables’s dirPath -> srcPath 
and findBestDiskAndInvalidCache’s path -> srcPath
fixed
bq. Minor/usability nit: I couldn’t find many cases where 
@Option(required=true) is used. WDYT about moving the path to a positional 
argument since its required and this command does not take a variable number of 
positional args?
makes sense, made it {{nodetool import <ks> <cf> <dir>}}
bq. Minor/usability nit: Instead of noVerify=true,noVerifyTokens=false being an 
invalid state, make noVerify=true imply noVerifyTokens=true. 
yup, makes sense
bq. The JavaDoc for CFS.loadNewSSTables should be updated to point to the new 
StorageService.loadSSTables. 
bq. The comment on CFS#L861 is useful but out of place. 
bq. Minor/naming nit: The naming of the “allKeys” variable in 
ImportTest#testImportInvalidateCache is misleading. 
bq. Instead of hardcoding token values what about using e.g. 
t.compareTo(mock.getDiskBoundaries().positions.get(0).getToken()) <= 0?
bq. Are you intentionally leaving the Random seed hardcoded?
fixed

> redesign loadnewsstables
> ------------------------
>
>                 Key: CASSANDRA-6719
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-6719
>             Project: Cassandra
>          Issue Type: New Feature
>          Components: Tools
>            Reporter: Jonathan Ellis
>            Assignee: Marcus Eriksson
>            Priority: Minor
>              Labels: lhf
>             Fix For: 4.x
>
>         Attachments: 6719.patch
>
>
> CFSMBean.loadNewSSTables scans data directories for new sstables dropped 
> there by an external agent.  This is dangerous because of possible filename 
> conflicts with existing or newly generated sstables.
> Instead, we should support leaving the new sstables in a separate directory 
> (specified by a parameter, or configured as a new location in yaml) and take 
> care of renaming as necessary automagically.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org

Reply via email to