You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by aweisberg <gi...@git.apache.org> on 2018/12/11 22:53:36 UTC

[GitHub] cassandra-dtest pull request #41: 14421

GitHub user aweisberg opened a pull request:

    https://github.com/apache/cassandra-dtest/pull/41

    14421

    

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/aweisberg/cassandra-dtest 14421

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/cassandra-dtest/pull/41.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #41
    
----
commit e13f9ace03f9fbcf0f302d2dfa54aad6d87cdf0b
Author: Ariel Weisberg <aw...@...>
Date:   2018-10-31T20:17:17Z

    Fix upgrade tests so they can at least run.
    
    Fix assertion
    
    Fix configuration generation in upgrade tests. Avoid using environment variables for cassandra directory and version.
    
    Maybe fix thrift bugs
    
    Fix some tests that use compact storage, update upgrade manifest to point to newer releases.
    
    Make some progress on sstable_generation_loading_test.py
    
    Bring storage_engine_update_test up to date
    
    Fix broken paging test that refers to the wrong version string.
    
    Add comment pointing out that some tests will still fail on MacOS.
    
    Bunch of cases where we need to reinit the config files after changing the cluster version.
    
    For upgrade through versions add testing for upcoming 4.0
    
    use my 3.0 so I can fix the migration bug.
    
    No more upgrade testing 2.0\!
    
    Use another of my own branches
    
    Update version string to handle non-cassandra branches
    
    Don't point to broken 3.11
    
    Use updated comparison function.
    
    Fix another assertion
    
    Fix the rest of the assertions.
    
    That assertion should be the old style.
    
    cql doesn't need encoding to binary
    
    Exception handling, fix assertions
    
    Missed a validation method
    
    Fix exception formatting
    
    Shouldn't run on 4.0
    
    Fix typo
    
    Really do need to know the CCM directory
    
    Fix various thrift tests.
    
    Will this fix it?
    
    Fix one of the tests
    
    Fix more of thrift_upgrade_test.py
    
    maybe fixed them?
    
    Much pain
    
    Uh, does schema take stupid time to propagate?
    
    fix test bug
    
    Handle 4.0 validation correctly
    
    Fix compact storage assertions in storage_engine_upgrade_test.py
    
    Some test cleanup
    
    Fix test_failure_threshold_deletions test

commit 9c1aee87a7fbf7474154365d27206e110e707f36
Author: Ariel Weisberg <aw...@...>
Date:   2018-12-11T22:15:38Z

    commit to play with fix to list_to_hashed_dict

----


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240823234
  
    --- Diff: dtest_setup.py ---
    @@ -454,22 +463,14 @@ def maybe_setup_jacoco(self, cluster_name='test'):
     
         @staticmethod
         def create_ccm_cluster(dtest_setup):
    -        logger.debug("cluster ccm directory: " + dtest_setup.test_path)
    +        logger.info("cluster ccm directory: " + dtest_setup.test_path)
    --- End diff --
    
    Does this need to be info level?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240827292
  
    --- Diff: upgrade_tests/storage_engine_upgrade_test.py ---
    @@ -140,6 +139,10 @@ def upgrade_with_clustered_table(self, compact_storage=False):
                 for r in range(ROWS):
                     session.execute("INSERT INTO t(k, t, v) VALUES ({n}, {r}, {r})".format(n=n, r=r))
     
    +        #4.0 doesn't support compact storage
    +        if compact_storage and self.dtest_config.cassandra_version_from_build > '4':
    --- End diff --
    
    Use {{LooseVersion}} here as well?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r243218570
  
    --- Diff: upgrade_tests/upgrade_through_versions_test.py ---
    @@ -794,31 +800,33 @@ def create_upgrade_class(clsname, version_metas, protocol_version,
     MultiUpgrade = namedtuple('MultiUpgrade', ('name', 'version_metas', 'protocol_version', 'extra_config'))
     
     MULTI_UPGRADES = (
    -    # Proto v3 upgrades (v3 is supported on 2.1, 2.2, 3.0, 3.1, trunk)
    -    MultiUpgrade(name='TestProtoV3Upgrade_AllVersions_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_2_1_x, current_2_2_x, current_3_0_x, indev_3_x], protocol_version=3, extra_config=None),
    -    MultiUpgrade(name='TestProtoV3Upgrade_AllVersions_RandomPartitioner_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_2_1_x, current_2_2_x, current_3_0_x, indev_3_x], protocol_version=3,
    +    # Proto v3 upgrades (v3 is supported on 2.1, 2.2, 3.0, 3.11)
    +    MultiUpgrade(name='TestProtoV3Upgrade_AllVersions_EndsAt_3_11_X',
    +                 version_metas=[current_2_1_x, current_2_2_x, current_3_0_x, indev_3_11_x], protocol_version=3, extra_config=None),
    +    MultiUpgrade(name='TestProtoV3Upgrade_AllVersions_RandomPartitioner_EndsAt_3_11_X_HEAD',
    +                 version_metas=[current_2_1_x, current_2_2_x, current_3_0_x, indev_3_11_x], protocol_version=3,
                      extra_config=(
                          ('partitioner', 'org.apache.cassandra.dht.RandomPartitioner'),
                      )),
     
         # Proto v4 upgrades (v4 is supported on 2.2, 3.0, 3.1, trunk)
         MultiUpgrade(name='TestProtoV4Upgrade_AllVersions_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_2_2_x, current_3_0_x, indev_3_x, ], protocol_version=4, extra_config=None),
    +                 version_metas=[current_2_2_x, current_3_0_x, current_3_11_x, indev_trunk], protocol_version=4, extra_config=None),
         MultiUpgrade(name='TestProtoV4Upgrade_AllVersions_RandomPartitioner_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_2_2_x, current_3_0_x, indev_3_x], protocol_version=4,
    +                 version_metas=[current_2_2_x, current_3_0_x, current_3_11_x, indev_trunk], protocol_version=4,
                      extra_config=(
                          ('partitioner', 'org.apache.cassandra.dht.RandomPartitioner'),
                      )),
    +    #Beta versions don't work with this test since it doesn't specify use beta in the client
    +    #It's fine I guess for now? Can update on release
         # Proto v5 upgrades (v5 is supported on 3.0, 3.11, trunk)
    -    MultiUpgrade(name='TestProtoV5Upgrade_AllVersions_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_3_0_x, current_3_x, indev_trunk], protocol_version=5, extra_config=None),
    -    MultiUpgrade(name='TestProtoV5Upgrade_AllVersions_RandomPartitioner_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_3_0_x, current_3_x, indev_trunk], protocol_version=5,
    -                 extra_config=(
    -                     ('partitioner', 'org.apache.cassandra.dht.RandomPartitioner'),
    -                 )),
    +    # MultiUpgrade(name='TestProtoV5Upgrade_AllVersions_EndsAt_Trunk_HEAD',
    --- End diff --
    
    Do we want to leave commented code in? It is ok if this is transient and we'll eventually remove it.


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240837053
  
    --- Diff: upgrade_tests/upgrade_manifest.py ---
    @@ -70,22 +89,19 @@ def clone_with_local_env_version(self):
             return self
     
     
    -indev_2_0_x = None  # None if release not likely
    -current_2_0_x = VersionMeta(name='current_2_0_x', family='2.0.x', variant='current', version='2.0.17', min_proto_v=1, max_proto_v=2, java_versions=(7,))
    -
     indev_2_1_x = VersionMeta(name='indev_2_1_x', family='2.1.x', variant='indev', version='github:apache/cassandra-2.1', min_proto_v=1, max_proto_v=3, java_versions=(7, 8))
    -current_2_1_x = VersionMeta(name='current_2_1_x', family='2.1.x', variant='current', version='2.1.17', min_proto_v=1, max_proto_v=3, java_versions=(7, 8))
    +current_2_1_x = VersionMeta(name='current_2_1_x', family='2.1.x', variant='current', version='2.1.20', min_proto_v=1, max_proto_v=3, java_versions=(7, 8))
     
     indev_2_2_x = VersionMeta(name='indev_2_2_x', family='2.2.x', variant='indev', version='github:apache/cassandra-2.2', min_proto_v=1, max_proto_v=4, java_versions=(7, 8))
    -current_2_2_x = VersionMeta(name='current_2_2_x', family='2.2.x', variant='current', version='2.2.9', min_proto_v=1, max_proto_v=4, java_versions=(7, 8))
    +current_2_2_x = VersionMeta(name='current_2_2_x', family='2.2.x', variant='current', version='2.2.13', min_proto_v=1, max_proto_v=4, java_versions=(7, 8))
     
    -indev_3_0_x = VersionMeta(name='indev_3_0_x', family='3.0.x', variant='indev', version='github:apache/cassandra-3.0', min_proto_v=3, max_proto_v=4, java_versions=(8,))
    -current_3_0_x = VersionMeta(name='current_3_0_x', family='3.0.x', variant='current', version='3.0.12', min_proto_v=3, max_proto_v=4, java_versions=(8,))
    +indev_3_0_x = VersionMeta(name='indev_3_0_x', family='3.0.x', variant='indev', version='github:aweisberg/cassandra-3.0', min_proto_v=3, max_proto_v=4, java_versions=(8,))
    --- End diff --
    
    Like I said in the JIRA I will update the branches on commit once https://issues.apache.org/jira/browse/CASSANDRA-14928 is merged.


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r246170593
  
    --- Diff: upgrade_tests/upgrade_through_versions_test.py ---
    @@ -794,31 +800,33 @@ def create_upgrade_class(clsname, version_metas, protocol_version,
     MultiUpgrade = namedtuple('MultiUpgrade', ('name', 'version_metas', 'protocol_version', 'extra_config'))
     
     MULTI_UPGRADES = (
    -    # Proto v3 upgrades (v3 is supported on 2.1, 2.2, 3.0, 3.1, trunk)
    -    MultiUpgrade(name='TestProtoV3Upgrade_AllVersions_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_2_1_x, current_2_2_x, current_3_0_x, indev_3_x], protocol_version=3, extra_config=None),
    -    MultiUpgrade(name='TestProtoV3Upgrade_AllVersions_RandomPartitioner_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_2_1_x, current_2_2_x, current_3_0_x, indev_3_x], protocol_version=3,
    +    # Proto v3 upgrades (v3 is supported on 2.1, 2.2, 3.0, 3.11)
    +    MultiUpgrade(name='TestProtoV3Upgrade_AllVersions_EndsAt_3_11_X',
    +                 version_metas=[current_2_1_x, current_2_2_x, current_3_0_x, indev_3_11_x], protocol_version=3, extra_config=None),
    +    MultiUpgrade(name='TestProtoV3Upgrade_AllVersions_RandomPartitioner_EndsAt_3_11_X_HEAD',
    +                 version_metas=[current_2_1_x, current_2_2_x, current_3_0_x, indev_3_11_x], protocol_version=3,
                      extra_config=(
                          ('partitioner', 'org.apache.cassandra.dht.RandomPartitioner'),
                      )),
     
         # Proto v4 upgrades (v4 is supported on 2.2, 3.0, 3.1, trunk)
         MultiUpgrade(name='TestProtoV4Upgrade_AllVersions_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_2_2_x, current_3_0_x, indev_3_x, ], protocol_version=4, extra_config=None),
    +                 version_metas=[current_2_2_x, current_3_0_x, current_3_11_x, indev_trunk], protocol_version=4, extra_config=None),
         MultiUpgrade(name='TestProtoV4Upgrade_AllVersions_RandomPartitioner_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_2_2_x, current_3_0_x, indev_3_x], protocol_version=4,
    +                 version_metas=[current_2_2_x, current_3_0_x, current_3_11_x, indev_trunk], protocol_version=4,
                      extra_config=(
                          ('partitioner', 'org.apache.cassandra.dht.RandomPartitioner'),
                      )),
    +    #Beta versions don't work with this test since it doesn't specify use beta in the client
    +    #It's fine I guess for now? Can update on release
         # Proto v5 upgrades (v5 is supported on 3.0, 3.11, trunk)
    -    MultiUpgrade(name='TestProtoV5Upgrade_AllVersions_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_3_0_x, current_3_x, indev_trunk], protocol_version=5, extra_config=None),
    -    MultiUpgrade(name='TestProtoV5Upgrade_AllVersions_RandomPartitioner_EndsAt_Trunk_HEAD',
    -                 version_metas=[current_3_0_x, current_3_x, indev_trunk], protocol_version=5,
    -                 extra_config=(
    -                     ('partitioner', 'org.apache.cassandra.dht.RandomPartitioner'),
    -                 )),
    +    # MultiUpgrade(name='TestProtoV5Upgrade_AllVersions_EndsAt_Trunk_HEAD',
    --- End diff --
    
    The code is waiting to be introduced. I want to leave it in. It's not a great situation that we have to manually update all these manifests periodically. This at least leaves some bread crumbs WRT to it being necessary.


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r246168266
  
    --- Diff: upgrade_tests/thrift_upgrade_test.py ---
    @@ -10,99 +10,182 @@
                                                ColumnParent, ConsistencyLevel,
                                                SlicePredicate, SliceRange)
     from thrift_test import _i64, get_thrift_client
    -from tools.assertions import assert_length_equal
    +from tools.assertions import assert_length_equal, assert_lists_of_dicts_equal
    +from tools.misc import wait_for_agreement, add_skip
     from .upgrade_base import UpgradeTester
     from .upgrade_manifest import build_upgrade_pairs
     
     since = pytest.mark.since
     logger = logging.getLogger(__name__)
     
     
    -def _create_dense_super_cf(name):
    -    return Cassandra.CfDef('ks', name, column_type='Super',
    +def _create_dense_super_cf(thrift, name):
    +    cfdef = Cassandra.CfDef('ks', name, column_type='Super',
                                key_validation_class='AsciiType',        # pk
                                comparator_type='AsciiType',             # ck
                                default_validation_class='AsciiType',    # SC value
                                subcomparator_type='LongType')           # SC key
    +    thrift.system_add_column_family(cfdef)
    +    wait_for_agreement(thrift)
     
     
    -def _create_sparse_super_cf(name):
    -    cd1 = ColumnDef('col1', 'LongType', None, None)
    -    cd2 = ColumnDef('col2', 'LongType', None, None)
    -    return Cassandra.CfDef('ks', name, column_type='Super',
    +def _create_sparse_super_cf(thrift, name):
    +    cd1 = ColumnDef('col1'.encode(), 'LongType', None, None)
    +    cd2 = ColumnDef('col2'.encode(), 'LongType', None, None)
    +    cfdef = Cassandra.CfDef('ks', name, column_type='Super',
                                column_metadata=[cd1, cd2],
                                key_validation_class='AsciiType',
                                comparator_type='AsciiType',
                                subcomparator_type='AsciiType')
    +    thrift.system_add_column_family(cfdef)
    +    wait_for_agreement(thrift)
     
     
    -def _validate_sparse_cql(cursor, cf='sparse_super_1', column1='column1', col1='col1', col2='col2', key='key'):
    +def unpack(list):
    --- End diff --
    
    Renamed to lst


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r245848691
  
    --- Diff: upgrade_tests/thrift_upgrade_test.py ---
    @@ -350,29 +469,41 @@ def test_sparse_supercolumn_with_renames(self):
             client.transport.open()
             client.set_keyspace('ks')
     
    -        cf = _create_sparse_super_cf('sparse_super_1')
    -        client.system_add_column_family(cf)
    +        _create_sparse_super_cf(client, 'sparse_super_1')
    +
    +        #The alter after was failing claiming the column family didn't exist.
    +        #This test is so slow and it's Thrift and going away "soon" and we
    +        #Don't run it on every commit so just sleep
    +        import time
    +        time.sleep(5)
    --- End diff --
    
    Do we want to sleep for a fixed duration or keep retrying patiently until it succeeds?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r245846048
  
    --- Diff: conftest.py ---
    @@ -409,13 +429,16 @@ def pytest_collection_modifyitems(items, config):
         This function is called upon during the pytest test collection phase and allows for modification
         of the test items within the list
         """
    -    if not config.getoption("--collect-only") and config.getoption("--cassandra-dir") is None:
    -        if config.getoption("--cassandra-version") is None:
    +    collect_only = config.getoption("--collect-only")
    +    cassandra_dir = config.getoption("--cassandra-dir")
    +    cassandra_version = config.getoption("--cassandra-version")
    +    if not collect_only and cassandra_dir  is None:
    --- End diff --
    
    Nit: Extra space `cassandra_dir  is`. Please fix on commit.


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r245846879
  
    --- Diff: sstable_generation_loading_test.py ---
    @@ -27,45 +28,68 @@ def fixture_add_additional_log_patterns(self, fixture_dtest_setup):
             fixture_dtest_setup.allow_log_errors = True
     
         upgrade_from = None
    -    compact = False
    +    test_compact = False
    +
    +    def compact(self):
    +        return self.fixture_dtest_setup.cluster.version() < MAJOR_VERSION_4 and self.test_compact
     
         def create_schema(self, session, ks, compression):
             create_ks(session, ks, rf=2)
    -        create_cf(session, "standard1", compression=compression, compact_storage=self.compact)
    +        create_cf(session, "standard1", compression=compression, compact_storage=self.compact())
             create_cf(session, "counter1", compression=compression, columns={'v': 'counter'},
    -                  compact_storage=self.compact)
    +                  compact_storage=self.compact())
     
         def test_sstableloader_compression_none_to_none(self):
    +        if self.__class__.__name__ != 'TestBasedSSTableLoader' and self.upgrade_from is None:
    --- End diff --
    
    Since these two lines are redundant. Perhaps refactor this into a small method and reuse?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240833917
  
    --- Diff: dtest_setup.py ---
    @@ -454,22 +463,14 @@ def maybe_setup_jacoco(self, cluster_name='test'):
     
         @staticmethod
         def create_ccm_cluster(dtest_setup):
    -        logger.debug("cluster ccm directory: " + dtest_setup.test_path)
    +        logger.info("cluster ccm directory: " + dtest_setup.test_path)
    --- End diff --
    
    I set it to info intentionally. It's annoying to not know where the cluster data is stored while working on dtests. If a test is hung you can't go look at the log output from a node. Well you can, you have to go to PS to find out what this directory is. But... if no nodes are running? Like if the startup options are bad? You are SOL.


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r245848375
  
    --- Diff: upgrade_tests/thrift_upgrade_test.py ---
    @@ -10,99 +10,182 @@
                                                ColumnParent, ConsistencyLevel,
                                                SlicePredicate, SliceRange)
     from thrift_test import _i64, get_thrift_client
    -from tools.assertions import assert_length_equal
    +from tools.assertions import assert_length_equal, assert_lists_of_dicts_equal
    +from tools.misc import wait_for_agreement, add_skip
     from .upgrade_base import UpgradeTester
     from .upgrade_manifest import build_upgrade_pairs
     
     since = pytest.mark.since
     logger = logging.getLogger(__name__)
     
     
    -def _create_dense_super_cf(name):
    -    return Cassandra.CfDef('ks', name, column_type='Super',
    +def _create_dense_super_cf(thrift, name):
    +    cfdef = Cassandra.CfDef('ks', name, column_type='Super',
                                key_validation_class='AsciiType',        # pk
                                comparator_type='AsciiType',             # ck
                                default_validation_class='AsciiType',    # SC value
                                subcomparator_type='LongType')           # SC key
    +    thrift.system_add_column_family(cfdef)
    +    wait_for_agreement(thrift)
     
     
    -def _create_sparse_super_cf(name):
    -    cd1 = ColumnDef('col1', 'LongType', None, None)
    -    cd2 = ColumnDef('col2', 'LongType', None, None)
    -    return Cassandra.CfDef('ks', name, column_type='Super',
    +def _create_sparse_super_cf(thrift, name):
    +    cd1 = ColumnDef('col1'.encode(), 'LongType', None, None)
    +    cd2 = ColumnDef('col2'.encode(), 'LongType', None, None)
    +    cfdef = Cassandra.CfDef('ks', name, column_type='Super',
                                column_metadata=[cd1, cd2],
                                key_validation_class='AsciiType',
                                comparator_type='AsciiType',
                                subcomparator_type='AsciiType')
    +    thrift.system_add_column_family(cfdef)
    +    wait_for_agreement(thrift)
     
     
    -def _validate_sparse_cql(cursor, cf='sparse_super_1', column1='column1', col1='col1', col2='col2', key='key'):
    +def unpack(list):
    +    result_list = []
    +    for item_dict in list:
    +        normalized_dict = {}
    +        for key, value in item_dict.items():
    +            if hasattr(value, "items"):
    +                assert(key == '')
    +                for a, b in value.items():
    +                    normalized_dict[a] = b
    +            else:
    +                normalized_dict[key] = value
    +        result_list.append(normalized_dict)
    +    return result_list
    +
    +
    +def add_value(list):
    --- End diff --
    
    Please rename `list` variable.


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240828717
  
    --- Diff: upgrade_tests/upgrade_manifest.py ---
    @@ -70,22 +89,19 @@ def clone_with_local_env_version(self):
             return self
     
     
    -indev_2_0_x = None  # None if release not likely
    -current_2_0_x = VersionMeta(name='current_2_0_x', family='2.0.x', variant='current', version='2.0.17', min_proto_v=1, max_proto_v=2, java_versions=(7,))
    -
     indev_2_1_x = VersionMeta(name='indev_2_1_x', family='2.1.x', variant='indev', version='github:apache/cassandra-2.1', min_proto_v=1, max_proto_v=3, java_versions=(7, 8))
    -current_2_1_x = VersionMeta(name='current_2_1_x', family='2.1.x', variant='current', version='2.1.17', min_proto_v=1, max_proto_v=3, java_versions=(7, 8))
    +current_2_1_x = VersionMeta(name='current_2_1_x', family='2.1.x', variant='current', version='2.1.20', min_proto_v=1, max_proto_v=3, java_versions=(7, 8))
     
     indev_2_2_x = VersionMeta(name='indev_2_2_x', family='2.2.x', variant='indev', version='github:apache/cassandra-2.2', min_proto_v=1, max_proto_v=4, java_versions=(7, 8))
    -current_2_2_x = VersionMeta(name='current_2_2_x', family='2.2.x', variant='current', version='2.2.9', min_proto_v=1, max_proto_v=4, java_versions=(7, 8))
    +current_2_2_x = VersionMeta(name='current_2_2_x', family='2.2.x', variant='current', version='2.2.13', min_proto_v=1, max_proto_v=4, java_versions=(7, 8))
     
    -indev_3_0_x = VersionMeta(name='indev_3_0_x', family='3.0.x', variant='indev', version='github:apache/cassandra-3.0', min_proto_v=3, max_proto_v=4, java_versions=(8,))
    -current_3_0_x = VersionMeta(name='current_3_0_x', family='3.0.x', variant='current', version='3.0.12', min_proto_v=3, max_proto_v=4, java_versions=(8,))
    +indev_3_0_x = VersionMeta(name='indev_3_0_x', family='3.0.x', variant='indev', version='github:aweisberg/cassandra-3.0', min_proto_v=3, max_proto_v=4, java_versions=(8,))
    --- End diff --
    
    This needs to be reverted?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240824209
  
    --- Diff: sstable_generation_loading_test.py ---
    @@ -141,6 +166,16 @@ def load_sstable_with_configuration(self, pre_compression=None, post_compression
                 session.execute("UPDATE standard1 SET v='{}' WHERE KEY='{}' AND c='col'".format(i, i))
                 session.execute("UPDATE counter1 SET v=v+1 WHERE KEY='{}'".format(i))
     
    +        #Will upgrade to a version that doesn't support compact storage so revert the compact
    +        #storage, this doesn't actually fix it yet
    +        if self.compact() and default_install_version >= '4':
    --- End diff --
    
    Use `LooseVersion` here as well? Prefer constants over magic strings / numbers.


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240827177
  
    --- Diff: upgrade_tests/paging_test.py ---
    @@ -503,22 +513,31 @@ def test_basic_compound_paging(self):
                 );
             """)
     
    -        cursor.execute("""
    -            CREATE TABLE test2 (
    -                k int,
    -                c1 int,
    -                c2 int,
    -                v text,
    -                PRIMARY KEY (k, c1, c2)
    -            ) WITH COMPACT STORAGE;
    -        """)
    +        testing_compact_storage = self.cluster.version() < '4.0'
    --- End diff --
    
    Use {{LooseVersion}} here as well?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240823966
  
    --- Diff: sstable_generation_loading_test.py ---
    @@ -27,45 +27,68 @@ def fixture_add_additional_log_patterns(self, fixture_dtest_setup):
             fixture_dtest_setup.allow_log_errors = True
     
         upgrade_from = None
    -    compact = False
    +    test_compact = False
    +
    +    def compact(self):
    +        return self.fixture_dtest_setup.cluster.version() < '4' and self.test_compact
    --- End diff --
    
    Use `LooseVersion` instead of a string?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240827110
  
    --- Diff: upgrade_tests/paging_test.py ---
    @@ -457,21 +458,30 @@ def test_basic_paging(self):
                 );
             """)
     
    -        cursor.execute("""
    -            CREATE TABLE test2 (
    -                k int,
    -                c int,
    -                v text,
    -                PRIMARY KEY (k, c)
    -            ) WITH COMPACT STORAGE;
    -        """)
    +        testing_compact_storage = self.cluster.version() < '4.0'
    --- End diff --
    
    Use {{LooseVersion}} here as well?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r245846129
  
    --- Diff: conftest.py ---
    @@ -409,13 +429,16 @@ def pytest_collection_modifyitems(items, config):
         This function is called upon during the pytest test collection phase and allows for modification
         of the test items within the list
         """
    -    if not config.getoption("--collect-only") and config.getoption("--cassandra-dir") is None:
    -        if config.getoption("--cassandra-version") is None:
    +    collect_only = config.getoption("--collect-only")
    +    cassandra_dir = config.getoption("--cassandra-dir")
    +    cassandra_version = config.getoption("--cassandra-version")
    +    if not collect_only and cassandra_dir  is None:
    +        if  cassandra_version is None:
    --- End diff --
    
    Nit: Extra space `if  cassandra_version`. Please fix on commit.


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r245848315
  
    --- Diff: upgrade_tests/thrift_upgrade_test.py ---
    @@ -10,99 +10,182 @@
                                                ColumnParent, ConsistencyLevel,
                                                SlicePredicate, SliceRange)
     from thrift_test import _i64, get_thrift_client
    -from tools.assertions import assert_length_equal
    +from tools.assertions import assert_length_equal, assert_lists_of_dicts_equal
    +from tools.misc import wait_for_agreement, add_skip
     from .upgrade_base import UpgradeTester
     from .upgrade_manifest import build_upgrade_pairs
     
     since = pytest.mark.since
     logger = logging.getLogger(__name__)
     
     
    -def _create_dense_super_cf(name):
    -    return Cassandra.CfDef('ks', name, column_type='Super',
    +def _create_dense_super_cf(thrift, name):
    +    cfdef = Cassandra.CfDef('ks', name, column_type='Super',
                                key_validation_class='AsciiType',        # pk
                                comparator_type='AsciiType',             # ck
                                default_validation_class='AsciiType',    # SC value
                                subcomparator_type='LongType')           # SC key
    +    thrift.system_add_column_family(cfdef)
    +    wait_for_agreement(thrift)
     
     
    -def _create_sparse_super_cf(name):
    -    cd1 = ColumnDef('col1', 'LongType', None, None)
    -    cd2 = ColumnDef('col2', 'LongType', None, None)
    -    return Cassandra.CfDef('ks', name, column_type='Super',
    +def _create_sparse_super_cf(thrift, name):
    +    cd1 = ColumnDef('col1'.encode(), 'LongType', None, None)
    +    cd2 = ColumnDef('col2'.encode(), 'LongType', None, None)
    +    cfdef = Cassandra.CfDef('ks', name, column_type='Super',
                                column_metadata=[cd1, cd2],
                                key_validation_class='AsciiType',
                                comparator_type='AsciiType',
                                subcomparator_type='AsciiType')
    +    thrift.system_add_column_family(cfdef)
    +    wait_for_agreement(thrift)
     
     
    -def _validate_sparse_cql(cursor, cf='sparse_super_1', column1='column1', col1='col1', col2='col2', key='key'):
    +def unpack(list):
    --- End diff --
    
    `list` is a Python keyword - https://docs.python.org/3/tutorial/datastructures.html#more-on-lists
    
    You should not use `list` as a variable name. Although Python doesn't complain it's not a good practice.


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240827199
  
    --- Diff: upgrade_tests/paging_test.py ---
    @@ -503,22 +513,31 @@ def test_basic_compound_paging(self):
                 );
             """)
     
    -        cursor.execute("""
    -            CREATE TABLE test2 (
    -                k int,
    -                c1 int,
    -                c2 int,
    -                v text,
    -                PRIMARY KEY (k, c1, c2)
    -            ) WITH COMPACT STORAGE;
    -        """)
    +        testing_compact_storage = self.cluster.version() < '4.0'
    +        if testing_compact_storage:
    +            cursor.execute("""
    +                CREATE TABLE test2 (
    +                    k int,
    +                    c1 int,
    +                    c2 int,
    +                    v text,
    +                    PRIMARY KEY (k, c1, c2)
    +                ) WITH COMPACT STORAGE;
    +            """)
    +
    +        version_string = self.upgrade_version_string()
    +        #4.0 doesn't support compact storage
    +        if version_string == 'trunk' or version_string >= '4.0':
    --- End diff --
    
    Use {{LooseVersion}} here as well?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r246168551
  
    --- Diff: upgrade_tests/thrift_upgrade_test.py ---
    @@ -350,29 +469,41 @@ def test_sparse_supercolumn_with_renames(self):
             client.transport.open()
             client.set_keyspace('ks')
     
    -        cf = _create_sparse_super_cf('sparse_super_1')
    -        client.system_add_column_family(cf)
    +        _create_sparse_super_cf(client, 'sparse_super_1')
    +
    +        #The alter after was failing claiming the column family didn't exist.
    +        #This test is so slow and it's Thrift and going away "soon" and we
    +        #Don't run it on every commit so just sleep
    +        import time
    +        time.sleep(5)
    --- End diff --
    
    Removed entirely. I added the wait for schema agreement stuff so it shouldn't be necesssary


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r245846732
  
    --- Diff: requirements.txt ---
    @@ -1,6 +1,6 @@
     -e git+https://github.com/datastax/python-driver.git@cassandra-test#egg=cassandra-driver
     # Used ccm version is tracked by cassandra-test branch in ccm repo. Please create a PR there for fixes or upgrades to new releases.
    --e git+https://github.com/riptano/ccm.git@cassandra-test#egg=ccm
    +-e git+https://github.com/aweisberg/ccm.git@cassandra-test#egg=ccm
    --- End diff --
    
    Could you ensure that you change this when outstanding changes in upstream `ccm` are merged?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by dineshjoshi <gi...@git.apache.org>.
Github user dineshjoshi commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r240827140
  
    --- Diff: upgrade_tests/paging_test.py ---
    @@ -457,21 +458,30 @@ def test_basic_paging(self):
                 );
             """)
     
    -        cursor.execute("""
    -            CREATE TABLE test2 (
    -                k int,
    -                c int,
    -                v text,
    -                PRIMARY KEY (k, c)
    -            ) WITH COMPACT STORAGE;
    -        """)
    +        testing_compact_storage = self.cluster.version() < '4.0'
    +        if testing_compact_storage:
    +            cursor.execute("""
    +                CREATE TABLE test2 (
    +                    k int,
    +                    c int,
    +                    v text,
    +                    PRIMARY KEY (k, c)
    +                ) WITH COMPACT STORAGE;
    +            """)
    +
    +        version_string = self.upgrade_version_string()
    +        #4.0 doesn't support compact storage
    +        if version_string == 'trunk' or version_string >= '4.0':
    --- End diff --
    
    Use {{LooseVersion}} here as well?


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r246167650
  
    --- Diff: requirements.txt ---
    @@ -1,6 +1,6 @@
     -e git+https://github.com/datastax/python-driver.git@cassandra-test#egg=cassandra-driver
     # Used ccm version is tracked by cassandra-test branch in ccm repo. Please create a PR there for fixes or upgrades to new releases.
    --e git+https://github.com/riptano/ccm.git@cassandra-test#egg=ccm
    +-e git+https://github.com/aweisberg/ccm.git@cassandra-test#egg=ccm
    --- End diff --
    
    fixed


---

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


[GitHub] cassandra-dtest pull request #41: 14421

Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:

    https://github.com/apache/cassandra-dtest/pull/41#discussion_r246167627
  
    --- Diff: sstable_generation_loading_test.py ---
    @@ -27,45 +28,68 @@ def fixture_add_additional_log_patterns(self, fixture_dtest_setup):
             fixture_dtest_setup.allow_log_errors = True
     
         upgrade_from = None
    -    compact = False
    +    test_compact = False
    +
    +    def compact(self):
    +        return self.fixture_dtest_setup.cluster.version() < MAJOR_VERSION_4 and self.test_compact
     
         def create_schema(self, session, ks, compression):
             create_ks(session, ks, rf=2)
    -        create_cf(session, "standard1", compression=compression, compact_storage=self.compact)
    +        create_cf(session, "standard1", compression=compression, compact_storage=self.compact())
             create_cf(session, "counter1", compression=compression, columns={'v': 'counter'},
    -                  compact_storage=self.compact)
    +                  compact_storage=self.compact())
     
         def test_sstableloader_compression_none_to_none(self):
    +        if self.__class__.__name__ != 'TestBasedSSTableLoader' and self.upgrade_from is None:
    --- End diff --
    
    Fixed by adding skip_base_class_test which calls pytest.skip with an explanation.


---

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