[jira] [Comment Edited] (CASSANDRA-17009) Sstableverify unit test operate on SSTables

["Berenguer Blasi (Jira)" <ji...@apache.org>]

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

Berenguer Blasi edited comment on CASSANDRA-17009 at 9/30/21, 6:44 AM:
-----------------------------------------------------------------------

[~bhouser] here is my first pass:
- I noted a few minor typos in the PR
- Sthg weird happened with the file {{cassandra.in.sh}}. Make sure you're branching off the right place etc
- I would use this opportunity to add {{assertOnCleanExit()}} on the already existing test when possible
- That bug on the return code I would fix in this ticket. Bear in mind CI is heavy and we'll need to ping more people to review this ticket. The less we go back and forth the better. So if we can pack it here, within reason, I'd do that. Seems to be It'd be a few loc only.
- The issue with the git ignore of {{test/data}} sounds weird to me it didn't came up sooner. I have to give it another thought.
- The already existing tests only 'scratch the surface'. They pass an argument and check the tool doesn't explode. But they don't check the sstables themselves to make sure the argument was effective. I.e does '-r' really have an effect?
- The new tests you added are really nice! but for completion purposes I would combine them with -q, -e, etc and any other params to make sure they all work with corrupted data i.e.
- We seem to be missing a -c test.

Wdyt, does this make sense to you? Sounds like a lot but what you did already is all the heavy lifting of coming up with the test. That is very nice. I think with a few additions we'd be there. Let me know what you think.


was (Author: bereng):
[~bhouser] here is my first pass:
- I noted a few minor typos in the PR
- Sthg weird happened with the file {cassandra.in.sh}}. Make sure you're branching off the right place etc
- I would use this opportunity to add {{assertOnCleanExit()}} on the already existing test when possible
- That bug on the return code I would fix in this ticket. Bear in mind CI is heavy and we'll need to ping more people to review this ticket. The less we go back and forth the better. So if we can pack it here, within reason, I'd do that. Seems to be It'd be a few loc only.
- The issue with the git ignore of {{test/data}} sounds weird to me it didn't came up sooner. I have to give it another thought.
- The already existing tests only 'scratch the surface'. They pass an argument and check the tool doesn't explode. But they don't check the sstables themselves to make sure the argument was effective. I.e does '-r' really have an effect?
- The new tests you added are really nice! but for completion purposes I would combine them with -q, -e, etc and any other params to make sure they all work with corrupted data i.e.
- We seem to be missing a -c test.

Wdyt, does this make sense to you? Sounds like a lot but what you did already is all the heavy lifting of coming up with the test. That is very nice. I think with a few additions we'd be there. Let me know what you think.

> Sstableverify unit test operate on SSTables
> -------------------------------------------
>
>                 Key: CASSANDRA-17009
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-17009
>             Project: Cassandra
>          Issue Type: Sub-task
>          Components: Tool/sstable
>            Reporter: Brian Houser
>            Assignee: Brian Houser
>            Priority: Normal
>          Time Spent: 2h
>  Remaining Estimate: 0h
>
> as part of https://issues.apache.org/jira/browse/CASSANDRA-16009, unit coverage is a bit lax and doesn't run through the verifier (based on my coverage results).
> There should be a unit test that exercises the internal verifier both for a corrupt example and a working example.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

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