Create and delete virtual multitenant tables by hhkwong · Pull Request #648 · salesforce/mt-dynamo

hhkwong · 2019-11-15T23:44:15Z

This is a first version. Notes:

The MtAmazonDynamoDb interface now has a new method, createMultitenantTable. It's supported only in MtAmazonDynamoDbBySharedTable.
To use this in MtAmazonDynamoDbBySharedTable, a top-level context must have been defined when constructing the client, and the context when calling createMultitenantTable must match that top-level context. Similarly, when deleting a multitenant table, the context must match the top-level context. (And the same thing when we support cross-tenant scans.)
createMultitenantTable() results in a new physical table being created on the fly. The fact that we now not only have static physical tables makes it harder to know what the set of physical tables is at any given time. What I have right now is to assume each shared table client has a unique table prefix, and determine the set of physical tables based on prefix. Not sure if this is a good idea.
Instead of storing TableDescription jsons in the table description repo, we now store MtTableDescription jsons, where MtTableDescription extends TableDescription and has an extra flag isMultitenant (and more things in the future). This should make the new logic still be able to parse existing table description repo records.
Backup of table description repo records still needs work (each record stored in S3 now needs the extra information in MtTableDescription) -- should be straightforward but I just haven't done it because it's in Kotlin which I'm not familiar with...
Thinking about how to have broad test coverage for MT tables, maybe similar to having a new MT strategy in test ArgumentBuilder

hhkwong · 2019-11-15T23:45:45Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/MtAmazonDynamoDbBase.java

+     * @return true if the given table name is a physical table associated with this instance, false otherwise.
     */
-    protected boolean isMtTable(String tableName) {
+    protected boolean isPhysicalTable(String tableName) {


To make this clear that by this we mean a physical table associated with the instance, as opposed to a virtual multitenant table.

hhkwong · 2019-11-15T23:48:17Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/CreateTableRequestFactory.java

+     * Takes a virtual table description and returns a CreateTableRequest for the corresponding physical table to be
+     * created dynamically.
+     */
+    CreateTableRequest getDynamicPhysicalTable(DynamoTableDescription virtualTableDescription);


This tells us how to create a new physical table on the fly when there's a request to create a new multitenant table

hhkwong · 2019-11-15T23:54:16Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/CreateTableRequestFactory.java

+    /**
+     * Returns whether the given table name is of a physical table (static or dynamic) belonging to this factory.
+     */
+    boolean isPhysicalTable(String tableName);


This is based on table prefix, with the assumption that each mt-dynamo client uses a unique table prefix. Before, we know the exact set of physical tables because we have only static tables. But now we have tables created dynamically / on the fly, so it becomes tricky for each server to keep up-to-date with the current set of physical tables.

hhkwong · 2019-11-15T23:55:41Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/SharedTableBuilder.java

        setDefaults();
        withName("SharedTableBuilder");
        validate();
+        PhysicalTableManager physicalTableManager = new PhysicalTableManager(amazonDynamoDb, pollIntervalSeconds);


Abstracted out physical table create/describe logic from TableMappingFactory into the new PhysicalTableManager

hhkwong · 2019-11-15T23:58:03Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/SharedTableBuilder.java

            CreateTableRequestFactory createTableRequestFactory = new SharedTableCreateTableRequestFactory(
-                partitioningStrategy.getTablePrimaryKeyMapper(), createTableRequests, getTablePrefix());
+                partitioningStrategy.getTablePrimaryKeyMapper(), createTableRequests,
+                partitioningStrategy::toCompatiblePhysicalPrimaryKey, tablePrefix,


TablePartitioningStrategy now has a new method toCompatiblePhysicalPrimaryKey() that converts a given virtual PK into a physical PK, so that when there's a request to create a new multitenant table, we know how to convert the virtual table CreateTableRequest into a physical one.

hhkwong · 2019-11-16T00:00:36Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/SharedTableBuilder.java

+        }
+
+        @Override
+        public CreateTableRequest getDynamicPhysicalTable(DynamoTableDescription virtualTable) {


This is called when we want to create a new multitenant table, which means we create a new physical table dynamically. To do this, we convert each table or secondary index PK of the virtual table into a physical PK, according to how the TablePartitioningStrategy defines the conversion.

hhkwong · 2019-11-16T00:04:51Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/impl/TableMappingFactory.java

+     * Creates the table mapping for a new virtual table, creating the physical table if it doesn't exist.
+     *
+     */
+    TableMapping createNewTableMapping(CreateTableRequest virtualCreateTableRequest, boolean isMultitenant) {


Now in addition to the existing getTableMapping(), we also have createNewTableMapping(). createNewTableMapping() is called when MtAmazonDynamoDbBySharedTable.createTable() or createMultitenantTable() is called, and creates the corresponding physical table if needed. getTableMapping() used to create the physical table if it doesn't exist, but now it doesn't -- it only calls describe.

Upon createTable, we used to only validate the virtual and physical table definitions, without creating the physical table if it doesn't exist. Now we make sure the physical table is created right away, instead of possibly waiting until the first DML.

hhkwong · 2019-11-16T00:09:32Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/impl/TableMappingFactory.java

+    private CreateTableRequest lookupPhysicalTable(VirtualDynamoTableDescription virtualTable) {
+        // if this is a multitenant table, then create a new physical table dedicated to this purpose.
+        // otherwise, find the corresponding static physical table.
+        return virtualTable.isMultitenant()


If this is a multitenant table, then create a dedicated physical table for it on the fly. Thought about making this not hard-coded here, and letting CreateTableRequestFactory decide whether to use one of the static tables or create a new physical table, but if we make it more flexible, then deleteTable becomes trickier (if the MT table doesn't necessarily have its own physical table, then we can't just delete the physical table).

hhkwong · 2019-11-16T00:13:53Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/SharedTableBuilder.java

    private String name;
    private AmazonDynamoDB amazonDynamoDb;
    private MtAmazonDynamoDbContextProvider mtContext;
+    private Optional<String> topLevelContext = empty();


A shared table client must define a top-level context to be able to create multitenant tables. Creating and deleting multitenant tables can be done only when the context matches the top-level context.

hhkwong · 2019-11-16T00:15:56Z

...ava/com/salesforce/dynamodbv2/mt/mappers/sharedtable/impl/MtAmazonDynamoDbBySharedTable.java

    @Override
-    protected boolean isMtTable(String tableName) {
-        return mtTables.containsKey(tableName) && !tableName.startsWith(backupTablePrefix);
+    protected boolean isPhysicalTable(String tableName) {


Again, this is now based on table prefix, instead of looking at the set of static physical tables, because now we have physical tables created on the fly. Need to exclude the table description repo table because that also has the same prefix but isn't a data table.

Is this filter going to match the *.Leases tables?

Oh yeah it is going to match that too... Maybe it'd be better to make dynamically created tables have an extra suffix to the prefix, e.g., [clientTablePrefix].d, or something like that, and look for only the static data tables and dynamic data tables found in this way.

What are the rules for mt-dynamo table names and prefixes? Are they defined somewhere?

hhkwong · 2019-11-16T00:17:13Z

...ava/com/salesforce/dynamodbv2/mt/mappers/sharedtable/impl/MtAmazonDynamoDbBySharedTable.java

+        this.physicalTableManager = physicalTableManager;
        this.deleteTableAsync = deleteTableAsync;
        this.truncateOnDeleteTable = truncateOnDeleteTable;
-        this.mtTables = tableMappingFactory.getCreateTableRequestFactory().getPhysicalTables().stream()


I think we don't need this because PhysicalTableManager already has a cache of the physical table descriptions that this server knows about (and that cache used to be in TableMappingFactory)

hhkwong · 2019-11-16T00:18:43Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/impl/PhysicalTableManager.java

+import org.slf4j.LoggerFactory;
+
+public class PhysicalTableManager {
+


Moved the create & describe table logic plus the physical table description cache from TableMappingFactory to here. Also now have a delete table method, called when we delete a multitenant table.

hhkwong · 2019-11-16T00:21:30Z

...ava/com/salesforce/dynamodbv2/mt/mappers/sharedtable/impl/MtAmazonDynamoDbBySharedTable.java

+    }
+
+    private CreateTableResult createTable(CreateTableRequest createTableRequest, boolean isMultitenant) {
+        TableMapping tableMapping = tableMappingFactory.createNewTableMapping(createTableRequest, isMultitenant);


TableMappingFactory.createNewTableMapping() is a new method on TableMappingFactory. It validates the request and creates the physical table if it doesn't already exist.
(We used to create the physical table lazily upon the first DML. TableMappingFactory.getTableMapping() used to create the physical table if it doesn't exist, but it no longer does this.)

hhkwong · 2019-11-16T00:22:51Z

...ava/com/salesforce/dynamodbv2/mt/mappers/sharedtable/impl/MtAmazonDynamoDbBySharedTable.java

-                deletedCount += scanResult.getItems().size();
-                if (scanResult.getLastEvaluatedKey() == null) {
-                    break;
+            if (tableDescription.isMultitenant()) {


If this is a multitenant table, then drop the corresponding physical table

hhkwong · 2019-11-16T00:27:58Z

...in/java/com/salesforce/dynamodbv2/mt/mappers/metadata/VirtualDynamoTableDescriptionImpl.java

+        this.isMultitenant = isMultitenant;
+    }
+
+    public VirtualDynamoTableDescriptionImpl(MtTableDescription tableDescription) {


MtTableDescription extends TableDescription, with an extra flag isMultitenant (and more properties in the future), and that's what we store in the table description repo now. This should make us still be able to parse existing TableDescription records.

hhkwong · 2019-11-16T00:29:24Z

src/main/java/com/salesforce/dynamodbv2/mt/repo/MtDynamoDbTableDescriptionRepo.java

    }

-    private TableDescription getTableDescriptionNoCache(String tableName) {
+    private MtTableDescription getTableDescriptionNoCache(String tableName) {


This is where we first try to get a description for the given virtual table name at the current context, and if it doesn't exist, see if there's virtual multitenant table with this name at the top-level context.

busjaeger · 2019-11-16T02:08:38Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/SharedTableBuilder.java

+
+        @Override
+        public CreateTableRequest getDynamicPhysicalTable(DynamoTableDescription virtualTable) {
+            String physicalTableName = prefix(tablePrefix, virtualTable.getTableName());


Do we need/want to enforce namespacing between the static physical tables configured on the adapter and the dynamic physical tables created for virtual mt tables? If not, what would happen if we create a virtual table with the same name as a static physical table?

Yes I think that's a good idea. Perhaps dynamic physical tables can have a different prefix, which would make isPhysicalTables() more robust / make it less likely that we accidentally pick up other tables like .leases tables.

busjaeger · 2019-11-16T02:10:05Z

...ava/com/salesforce/dynamodbv2/mt/mappers/sharedtable/impl/MtAmazonDynamoDbBySharedTable.java


    private final String name;
-
+    private final Optional<String> topLevelContext;


What is the purpose of this context?

This is so that we know whether we're allowed to create/delete/update a multitenant table or to do a cross-tenant scan on a multitenant table. For example this would be the Core cloud context.

Is this different from "no context" (as used in master for cross-tenant scans or change streaming)?

Yes this is different from "no context". It seems like when for example the Core cloud does DDL on a virtual multitenant table, or does a cross-tenant scan on a virtual mt table, there would still be a context.
But maybe we shouldn't make the distinction -- that would make things simpler.

busjaeger · 2019-11-16T02:13:04Z

src/main/java/com/salesforce/dynamodbv2/mt/mappers/sharedtable/impl/TableMappingFactory.java

+        // table. set the returned physical table description back onto the table mapping, so it includes things that
+        // can only be determined after the physical table is created, like the streamArn.
+        physicalTable = createPhysicalTableIfNotExists
+            ? physicalTableManager.createTableIfNotExists(physicalCreateTableRequest)


Are we creating physical tables synchronously here?

Yes it's still synchronous

busjaeger · 2019-11-16T02:18:31Z

src/main/java/com/salesforce/dynamodbv2/mt/repo/MtDynamoDbTableDescriptionRepo.java

-    private TableDescription getTableDescriptionNoCache(String tableName) {
+    private MtTableDescription getTableDescriptionNoCache(String tableName) {
+        // first get description for table name at the current context
+        Optional<MtTableDescription> tableDescription = getTableDescriptionFromDb(tableName);


Do we need/want to namespace virtual mt tables and virtual tables? If not, would tenant-level tables shadow mt tables?

To me it seems perhaps more confusing for a user to be able to define both a virtual mt table and a virtual tenant-level table with the same name, so I went with making them share the same namespace. What do you mean by tenant-level tables shadowing mt tables?

…s, more tests

…me as global

codecov-io · 2019-12-02T09:48:14Z

Codecov Report

Merging #648 into master will decrease coverage by 0.15%.
The diff coverage is 89.15%.

@@             Coverage Diff              @@
##             master     #648      +/-   ##
============================================
- Coverage     83.17%   83.02%   -0.16%     
- Complexity      987     1021      +34     
============================================
  Files            67       70       +3     
  Lines          4346     4437      +91     
  Branches        534      551      +17     
============================================
+ Hits           3615     3684      +69     
- Misses          478      493      +15     
- Partials        253      260       +7

Impacted Files	Coverage Δ	Complexity Δ
...namodbv2/mt/mappers/MtAmazonDynamoDbByAccount.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...force/dynamodbv2/mt/repo/MtTableDescriptionRepo.kt	`76.47% <100%> (-11.77%)`	`0 <0> (ø)`
.../dynamodbv2/mt/admin/AmazonDynamoDbAdminUtils.java	`69.11% <100%> (+1.42%)`	`15 <1> (+1)`	⬆️
...namodbv2/mt/mappers/MtAmazonDynamoDbComposite.java	`97.77% <100%> (+0.05%)`	`24 <1> (+1)`	⬆️
...modbv2/mt/mappers/MtAmazonDynamoDbStreamsBase.java	`78.4% <100%> (ø)`	`17 <1> (ø)`	⬇️
...namodbv2/mt/mappers/CreateTableRequestBuilder.java	`98.63% <100%> (+0.03%)`	`33 <1> (+1)`	⬆️
...v2/mt/context/MtAmazonDynamoDbContextProvider.java	`93.33% <100%> (ø)`	`4 <1> (ø)`	⬇️
...ers/sharedtable/impl/HashPartitioningStrategy.java	`91.66% <100%> (+0.75%)`	`10 <1> (+1)`	⬆️
...dynamodbv2/mt/mappers/MtAmazonDynamoDbByTable.java	`90.98% <100%> (ø)`	`26 <0> (ø)`	⬇️
...va/com/salesforce/dynamodbv2/mt/cache/MtCache.java	`44.44% <100%> (+5.55%)`	`5 <1> (+1)`	⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea79c3f...49eedf1. Read the comment docs.

Create and delete virtual multitenant tables

4b64866

hhkwong commented Nov 15, 2019

View reviewed changes

hhkwong commented Nov 16, 2019

View reviewed changes

hhkwong requested review from busjaeger, msgroi and sbabu-salesforce November 16, 2019 00:42

busjaeger reviewed Nov 16, 2019

View reviewed changes

hhkwong added 3 commits November 22, 2019 14:10

Namespace virtual mt tables, namesapce dynamic tables, unbreak backup…

9326797

…s, more tests

fix kotlin codestyle stuff

ab7b23e

make table naming rules consumer-configured; merge null contex the sa…

49eedf1

…me as global

		import org.slf4j.LoggerFactory;

		public class PhysicalTableManager {


		private final String name;

		private final Optional<String> topLevelContext;

Conversation

hhkwong commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hhkwong Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hhkwong Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hhkwong commented Nov 15, 2019 •

edited

Loading

hhkwong Nov 16, 2019 •

edited

Loading

hhkwong Nov 16, 2019 •

edited

Loading

codecov-io commented Dec 2, 2019 •

edited

Loading