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