New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test for materialized views implementation (CASSANDRA-6477) #360

Merged
merged 1 commit into from Jul 30, 2015

Conversation

Projects
None yet
3 participants
@aboudreault
Contributor

aboudreault commented Jul 2, 2015

Do not merge, but ready for some review.

Test for materialized views implementation.

https://issues.apache.org/jira/browse/CASSANDRA-6477

I am waiting on 2 fixes/changes to complete this PR:

  • Adding a datacenter or a single node produce unexpected results. (Bad data propagation)
  • The drop table and drop column has to be changed to NOT drop the materialized views silently. (see ticket for more info)
Show outdated Hide outdated materialized_views_test.py Outdated
@aboudreault

This comment has been minimized.

Show comment
Hide comment
@aboudreault

aboudreault Jul 7, 2015

Contributor

@mambocab When you have some time, I would appreciate a first review here :)

Contributor

aboudreault commented Jul 7, 2015

@mambocab When you have some time, I would appreciate a first review here :)

"""
Test materialized views implementation.
@jira_ticket CASSANDRA-6477
@since 3.0

This comment has been minimized.

@mambocab

mambocab Jul 7, 2015

Contributor

Do we have a standard meaning for @since?

@mambocab

mambocab Jul 7, 2015

Contributor

Do we have a standard meaning for @since?

This comment has been minimized.

@ptnapoleon

ptnapoleon Jul 7, 2015

Contributor

No :(

@ptnapoleon

ptnapoleon Jul 7, 2015

Contributor

No :(

Show outdated Hide outdated materialized_views_test.py Outdated
Show outdated Hide outdated materialized_views_test.py Outdated
Show outdated Hide outdated materialized_views_test.py Outdated
@mambocab

This comment has been minimized.

Show comment
Hide comment
@mambocab

mambocab Jul 7, 2015

Contributor

I find the use of and relationship between the prepare and prepare_user_table a little confusing. I think there's sort of already a conventional meaning for the method name prepare in the codebase, which prepare_user_table subverts. If it were me, I'd add a kwarg (maybe named user_table) to prepare and only create that table if the kwarg is truthy.

That's just me being a big ol' nerd, though, and if you disagree don't change anything.

Contributor

mambocab commented Jul 7, 2015

I find the use of and relationship between the prepare and prepare_user_table a little confusing. I think there's sort of already a conventional meaning for the method name prepare in the codebase, which prepare_user_table subverts. If it were me, I'd add a kwarg (maybe named user_table) to prepare and only create that table if the kwarg is truthy.

That's just me being a big ol' nerd, though, and if you disagree don't change anything.

@mambocab mambocab referenced this pull request Jul 8, 2015

Merged

Tests for CASSANDRA-7392 #371

@aboudreault

This comment has been minimized.

Show comment
Hide comment
@aboudreault

aboudreault Jul 20, 2015

Contributor

sounds good, will modify that prepare method.

Contributor

aboudreault commented Jul 20, 2015

sounds good, will modify that prepare method.

@aboudreault

This comment has been minimized.

Show comment
Hide comment
@aboudreault

aboudreault Jul 22, 2015

Contributor

@mambocab I pushed more changes since yesterday, your comments/review are welcome.

Contributor

aboudreault commented Jul 22, 2015

@mambocab I pushed more changes since yesterday, your comments/review are welcome.

@ptnapoleon

This comment has been minimized.

Show comment
Hide comment
@ptnapoleon

ptnapoleon Jul 22, 2015

Contributor

You use a lot of the python assert a == b, msg syntax, rather than the unittest self.assertEqual(a, b, msg) which is preferred.

Contributor

ptnapoleon commented Jul 22, 2015

You use a lot of the python assert a == b, msg syntax, rather than the unittest self.assertEqual(a, b, msg) which is preferred.

@aboudreault aboudreault changed the title from (WIP) Test for materialized views implementation (CASSANDRA-6477) to Test for materialized views implementation (CASSANDRA-6477) Jul 30, 2015

@aboudreault

This comment has been minimized.

Show comment
Hide comment
@aboudreault

aboudreault Jul 30, 2015

Contributor

Ready to merge.

Big thanks to @ptnapoleon and @tjake for their multiple contributions in that big PR.

Contributor

aboudreault commented Jul 30, 2015

Ready to merge.

Big thanks to @ptnapoleon and @tjake for their multiple contributions in that big PR.

aboudreault added a commit that referenced this pull request Jul 30, 2015

Merge pull request #360 from riptano/materialized-views-test
Test for materialized views implementation (CASSANDRA-6477)

@aboudreault aboudreault merged commit a1b3ff2 into master Jul 30, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ptnapoleon ptnapoleon deleted the materialized-views-test branch Jan 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment