cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jordan West (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (CASSANDRA-6719) redesign loadnewsstables
Date Fri, 06 Apr 2018 23:23:00 GMT

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

Jordan West edited comment on CASSANDRA-6719 at 4/6/18 11:22 PM:
-----------------------------------------------------------------

[~krummas], patch looks great. comments below (mostly minor):
 * Since {{nodetool refresh}} is being deprecated, instead of modified, it would be nice
to maintain as much of the same functionality as before. Additional options could be introduced
to address the two areas I noticed changes:
 ** FSUtils.handleCorruptSSTable/handleFSError are no longer called
 ** Row cache invalidation was not previously performed — this is a good thing regardless,
so maybe skip an option for this one. 

 * If using {{nodetool refresh}} with JBOD, the counting keys per boundary work is done just
to throw it away.
 * Minor/naming nit: consider renaming {{CFS#loadSSTables}}’s dirPath -> srcPath and
{{findBestDiskAndInvalidCache}}’s path -> srcPath
 * 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?
 * Minor/usability nit: Instead of noVerify=true,noVerifyTokens=false being an invalid state,
make noVerify=true imply noVerifyTokens=true. 
 * The JavaDoc for {{CFS.loadNewSSTables}} should be updated to point to the new {{StorageService.loadSSTables}}. 
 * The comment on CFS#L861 is useful but out of place. 
 * Minor/naming nit: The naming of the “allKeys” variable in {{ImportTest#testImportInvalidateCache}} is
misleading. 
 * Minor nits in {{ImportTest#testBestDisk}}:
 ** Instead of hardcoding token values what about using e.g. {{t.compareTo(mock.getDiskBoundaries().positions.get(0).getToken())
<= 0}}?
 ** Are you intentionally leaving the Random seed hardcoded?


was (Author: jrwest):
[~krummas], patch looks great. comments below (mostly minor):
 * Since {{nodetool refresh}} is being deprecated, instead of modified, it would be nice
to maintain as much of the same functionality as before. Additional options could be introduced
to address the two areas I noticed changes:
 ** FSUtils.handleCorruptSSTable/handleFSError are no longer called
 ** Row cache invalidation was not previously performed — this is a good thing regardless,
so maybe skip an option for this one. 

 * If using {{nodetool refresh}} with JDOB, the counting keys per boundary work is done just
to throw it away.
 * Minor/naming nit: consider renaming {{CFS#loadSSTables}}’s dirPath -> srcPath and
{{findBestDiskAndInvalidCache}}’s path -> srcPath
 * 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?
 * Minor/usability nit: Instead of noVerify=true,noVerifyTokens=false being an invalid state,
make noVerify=true imply noVerifyTokens=true. 
 * The JavaDoc for {{CFS.loadNewSSTables}} should be updated to point to the new {{StorageService.loadSSTables}}. 
 * The comment on CFS#L861 is useful but out of place. 
 * Minor/naming nit: The naming of the “allKeys” variable in {{ImportTest#testImportInvalidateCache}} is
misleading. 
 * Minor nits in {{ImportTest#testBestDisk}}:
 ** Instead of hardcoding token values what about using e.g. {{t.compareTo(mock.getDiskBoundaries().positions.get(0).getToken())
<= 0}}?
 ** Are you intentionally leaving the Random seed hardcoded?

> 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-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org


Mime
View raw message