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