ccmlib/scylla_node: add validate_sstable() and validate_checksums_sstable() helpers

[tchaikov]

this series adds

• validate_sstable()
• validate_checksums_sstable()

helpers to ScyllaNode

[tchaikov]

this change depends on https://github.com/scylladb/scylladb/pull/16106

[tchaikov]

Lgtm

But it, looks a bit excessive that we duplicate almost the exact code again and again, just for docs

yes, a little bit. but if i were (well, i am) the developer who is using this helper, i would be interested in the dict's schema, without the document link, i would have to search for it. by providing the link, we save the extra hop, and ensure that developers have the right reference. so the docstring should be important for us. regarding to the helper, it's pretty straightforward. and i agree it's kind of "excessive". but the document offsets this i guess.

[tchaikov]

v2:

• s/validate_checksums_sstable/validate_sstable_checksums/

[tchaikov]

LGTM, one small naming comment.

On a related note, @tchaikov should we add a with compaction disabled wrappers to all these methods, disabling compaction for the examined table and the schema tables? Otherwise, we will have to add the same to all call-sites, as the tests start to become flaky. No need to do it in this PR, just remembered it while reviewing this PR.

we have a disable_autocompaction . does this fulfill our needs? see https://github.com/scylladb/scylla-dtest/blob/42dd3a8a8f1a8bfd42865098353194f05a8eb009/tools/context.py#L87

[fruch]

LGTM, one small naming comment.

On a related note, @tchaikov should we add a with compaction disabled wrappers to all these methods, disabling compaction for the examined table and the schema tables? Otherwise, we will have to add the same to all call-sites, as the tests start to become flaky. No need to do it in this PR, just remembered it while reviewing this PR.

we have a disable_autocompaction . does this fulfill our needs? see https://github.com/scylladb/scylla-dtest/blob/42dd3a8a8f1a8bfd42865098353194f05a8eb009/tools/context.py#L87

Yes, it was built exactly for that use case of tools reading sstables

[denesb]

Yes, we started using it at some of the callers of these ccmlib methods, but I think we should move those inside them, as disabling compaction is not optional.

[tchaikov]

Yes, we started using it at some of the callers of these ccmlib methods, but I think we should move those inside them, as disabling compaction is not optional.

i'd prefer keeping this context manager out of these helpers, even we always use them together. i agree it'd be more convenient, but a tool should only do what it suppose to do. and the ccmlib is a relatively low-level library. at least that's my understanding.

[denesb]

Yes, we started using it at some of the callers of these ccmlib methods, but I think we should move those inside them, as disabling compaction is not optional.

i'd prefer keeping this context manager out of these helpers, even we always use them together. i agree it'd be more convenient, but a tool should only do what it suppose to do. and the ccmlib is a relatively low-level library. at least that's my understanding.

We can introduce one here, said context manager doesn't do anything high level. I don't think it is a good idea to make callers do something that is required for correct operation of a function.