[ https://issues.apache.org/jira/browse/CASSANDRA-16818?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17396054#comment-17396054 ]
Andres de la Peña edited comment on CASSANDRA-16818 at 8/9/21, 1:56 PM: ------------------------------------------------------------------------ We don't check the schema loading when {{loadsSchema}} is {{true}} because we can't be sure about whether the {{Schema}} class is going to be loaded when we load the table metadata. However, in the cases where {{loadsSchema}} is {{false}} we are sure that the {{TableMetadata}} isn't going to be loaded, so we can check that the schema isn't really loaded. If this weren't right we wouldn't be finding the test failure that concerns us. I agree that there isn't a difference between calling {{assertPostTestEnv}} with {{true}} or {{false}} in the cases where the schema is loaded, but there would be a difference if the schema would be unexpectedly loaded in those cases, which is what we are trying to test. Also, for the cases that currently set {{loadsSchema}} to {{true}} there is a more obvious difference since they would just fail if we passed {{false}} instead. In the end, a {{false}} {{loadsSchema}} parameter means that we are sure that the schema class shouldn't be loaded, and a {{true}} {{loadsSchema}} means that it may or may not load the schema. So maybe we could just rename the argument to {{maybeLoadsSchema}}, and add some JavaDoc about what it does and why it's necessary to run the tests in the right order, wdyt? was (Author: adelapena): We don't check the schema loading when {{loadsSchema}} is {{true}} because we can't be sure about whether the {{Schema}} class is going to be loaded when we load the table metadata. However, in the cases where we {{loadsSchema}} is {{false}} we are sure that the {{TableMetadata}} isn't going to be loaded, so we can check that the schema isn't really loaded. If it weren't true we wouldn't be finding the test failure that concerns us. I agree that there isn't a difference between calling {{assertPostTestEnv}} with {{true}} or {{false}} in the cases where the schema is loaded, but there would be a difference if the schema would be unexpectedly loaded in those cases, which is what we are trying to test. Also, for the cases that currently pass {{true}} to {{loadsSchema}}, there is a more obvious difference since they would just fail if we passed {{false}} instead. I n the end, a {{false}} {{loadsSchema}} parameter means that we are sure that the schema class shouldn't be loaded, and a {{true}} {{loadsSchema}} means that it may or may not load the schema. So maybe we could just rename the argument to {{maybeLoadsSchema}}, and add some JavaDoc about what it does and why it's necessary to run the tests in the right order, wdyt? > Fix usage instructions about sstabledump -k and -x options > ---------------------------------------------------------- > > Key: CASSANDRA-16818 > URL: https://issues.apache.org/jira/browse/CASSANDRA-16818 > Project: Cassandra > Issue Type: Bug > Components: Tool/sstable > Reporter: Andres de la Peña > Assignee: Andres de la Peña > Priority: Normal > Fix For: 4.0.1, 4.1 > > Time Spent: 20m > Remaining Estimate: 0h > > The options {{-k}} and {{-x}} of {{sstabledump}} admit multiple arguments, so > users can include or exclude multiple partitions. The intended usage is, for > example: > {code:java} > $ sstablepartitions <sstable_path> -k 1 > $ sstablepartitions <sstable_path> -k 1 2 3 > {code} > However, the following command will fail: > {code:java} > $ sstablepartitions -k 1 <sstable_path> > You must supply exactly one sstable > usage: sstabledump <sstable file path> <options> > Dump contents of given SSTable to standard output in JSON format. > -d CQL row per line internal representation > -e enumerate partition keys only > -k <arg> Partition key > -t Print raw timestamps instead of iso8601 date strings > -x <arg> Excluded partition key > {code} > This command fails because the sstable path is considered a fourth partition > key, and the mandatory argument for the sstable path is missed. While I think > this behaviour is correct, it can be a bit confusing for users, especially > when the information about usage describes both {{-k}} and {{-x}} as > single-argument. > I think that at least we should fix the description of the options to > indicate that there could be multiple included/excluded keys, and probably > improve the message about the missing sstable path when those options are > used. > Alternatively we could modify the options to have a single argument and allow > to repeat them, so we could accept things like: > {code:java} > $ sstablepartitions -k 1 <sstable_path> > $ sstablepartitions -k 1 -k 2 -k 3 <sstable_path> > $ sstablepartitions <sstable_path> -k 1 -k 2 -k 3 > {code} > The main downside of the latter approach is that the change in the syntax of > the command might cause compatibility issues. Also we would need to upgrade > {{commons-cli}} to at least 1.2 due to CLI-137. -- This message was sent by Atlassian Jira (v8.3.4#803005) --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org