Skip to content
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

Add virtual tables for CASSANDRA-7622 #205

Closed
wants to merge 1 commit into from

Conversation

clohfink
Copy link
Contributor

No description provided.

Copy link
Contributor

@zznate zznate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rolled through this pretty quickly with some high-level nits, bits and pieces. Will try to go back through at a design level soon (after re-reading ticket)

this.isVirtual = false;
this.klass = null;

assert isVirtual == (klass != null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorta OT, but we should just stop doing this and use Preconditions to throw IAEs. I hate the fact that a user can disable so much of our error checking w. a command line arg.

@@ -267,12 +267,18 @@ public boolean updatesRegularRows()
// columns is if we set some static columns, and in that case no clustering
// columns should be given. So in practice, it's enough to check if we have
// either the table has no clustering or if it has at least one of them set.
return metadata().clusteringColumns().isEmpty() || restrictions.hasClusteringColumnsRestrictions();
return !metadata().isVirtual() &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why use the method if we have it locally (and it's public anyhoo). The object reference is being used elsewhere herein already.

@@ -331,8 +337,11 @@ private Keyspace(String keyspaceName, boolean loadSSTables)
this.viewManager = new ViewManager(this);
for (TableMetadata cfm : metadata.tablesAndViews())
{
logger.trace("Initializing {}.{}", getName(), cfm.name);
initCf(Schema.instance.getTableMetadataRef(cfm.id), loadSSTables);
logger.info("Initializing {}.{}", getName(), cfm.name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change from trace() to info() intentional?


public static Key createKey()
{
return new Key();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to allow empty key creation as API? If not, switch it to a builder so's it can be validated.

@@ -529,4 +529,9 @@ public void checkComparable()
{
return testAssignment(receiver.type);
}

public ByteBuffer unsafeDecompose(Object object)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno... that a pretty shoot-self-in-foot thing to add the the marshalling API. What's the reasoning?


private SystemInfoKeyspace() {}

public static final String SETTINGS = "settings";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay! statics!

"estimatedColumnCountHistogram");
private CompositeType keyType;

public static Map<String, CQL3Type> columns()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above. Initialize statically.

CompactionManager.instance.interruptCompactionFor(Collections.singleton(metadata), true);
if (DatabaseDescriptor.isAutoSnapshot())
cfs.snapshot(Keyspace.getTimestampedSnapshotNameWithPrefix(cfs.name, ColumnFamilyStore.SNAPSHOT_DROP_PREFIX));
CommitLog.instance.forceRecycleAllSegments(Collections.singleton(metadata.id));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go back and look through comments, but can i drop a VT?

else
virtualKlass = null;

assert isVirtual == flags.contains(Flag.VIRTUAL) || !Strings.isNullOrEmpty(virtualKlass) && !flags.contains(Flag.VIRTUAL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above w. the asserts. Let's just draw a line in the sand and stop doing this.

&& indexes.equals(tm.indexes)
&& triggers.equals(tm.triggers)
&& isVirtual == tm.isVirtual
&& (virtualKlass == null || virtualKlass.equals(tm.virtualKlass));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null class a legal state?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants