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

Alternator: missing fields in return of DescribeTable #5026

Closed
nyh opened this issue Sep 15, 2019 · 7 comments
Closed

Alternator: missing fields in return of DescribeTable #5026

nyh opened this issue Sep 15, 2019 · 7 comments
Labels
area/alternator Alternator related Issues user request
Milestone

Comments

@nyh
Copy link
Contributor

nyh commented Sep 15, 2019

The DescribeTable operation returns only a subset of the fields it's supposed to return. While this is enough for many use cases - which only use DescribeTable to tell when creating a table actually finished - it might not be enough in different use cases.

What is missing - with xfailing tests for each already in alternator-test/test_describe_table.py:

  1. The table's schema in KeySchema, AttributeDefinitions (see test_describe_table_schema).
  2. ItemCount (see test_describe_table_item_count).
  3. TableSizeBytes (see test_describe_table_size).
  4. ProvisionedThroughput (see test_describe_table_provisioned_throughput).
  5. ARN (see test_describe_table_arn)
  6. TableId (see test_describe_table_id)

Also CreationDateTime is returned incorrectly (see separate issue #5013).

Also BillingModeSummary we return is always the same thing (PAY_PER_REQUEST) regardless of what the user chose when creating the table. This is often good enough, but we should reconsider this.

Moreover, when there are indexes in a table (GSI or LSI), we only provide partial information for those too - see xfailing test test_gsi_describe (we should probably have a similar test for LSI as well).

The DeleteTable operation returns the return type as DescribeTable, so maybe should return additional values as well.

@nyh nyh added the area/alternator Alternator related Issues label Sep 15, 2019
@tzach
Copy link
Contributor

tzach commented Sep 16, 2019

@nyh I do see

"TableId": "dbfd89f0-d86a-11e9-b3a5-000000000002"

On the Alternator create table response

@slivne slivne added this to the 3.x milestone Sep 22, 2019
@nyh
Copy link
Contributor Author

nyh commented Nov 20, 2019

@tzach reported to the mailing list a case where the AttributeDefinitions field was expected and caused an application to fail:

dynamodb-admin is an open-source node base UI project for DynamoDB.
I was able to create a table with it, but I try to view the data, I got the following error message below. Scylla itself does not report any issue.

TypeError: /usr/lib/node_modules/dynamodb-admin/views/scan.ejs:106
    104|     >
    105|       <datalist id="attributes">
 >> 106|         <% for (let AttributeDefinition of Table.AttributeDefinitions) { %>
    107|           <option value="<%= AttributeDefinition.AttributeName %>" />
    108|         <% } %>
    109|       </datalist>

Table.AttributeDefinitions is not iterable
    at eval (/usr/lib/node_modules/dynamodb-admin/views/scan.ejs:59:46)
    at scan (/usr/lib/node_modules/dynamodb-admin/node_modules/ejs/lib/ejs.js:682:17)
    at tryHandleCache (/usr/lib/node_modules/dynamodb-admin/node_modules/ejs/lib/ejs.js:254:36)
    at View.exports.renderFile [as engine] (/usr/lib/node_modules/dynamodb-admin/node_modules/ejs/lib/ejs.js:485:10)
    at View.render (/usr/lib/node_modules/dynamodb-admin/node_modules/express/lib/view.js:135:8)
    at tryRender (/usr/lib/node_modules/dynamodb-admin/node_modules/express/lib/application.js:640:10)
    at Function.render (/usr/lib/node_modules/dynamodb-admin/node_modules/express/lib/application.js:592:3)
    at ServerResponse.render (/usr/lib/node_modules/dynamodb-admin/node_modules/express/lib/response.js:1012:7)
    at describeTable.then.description (/usr/lib/node_modules/dynamodb-admin/lib/backend.js:404:13)
    at process._tickCallback (internal/process/next_tick.js:68:7)

@nyh nyh assigned nyh and unassigned nyh Nov 20, 2019
psarna pushed a commit that referenced this issue Nov 21, 2019
One of the fields still missing in DescribeTable's response (Refs #5026)
was the table's schema - KeySchema and AttributeDefinitions.

This patch adds this missing feature, and enables the previously-xfailing
test test_describe_table_schema.

A complication of this patch is that in a table with secondary indexes,
we need to return not just the base table's schema, but also the indexes'
schema. The existing tests did not cover that feature, so we add here
two more tests in test_gsi.py for that.

One of these secondary-index schema tests, test_gsi_2_describe_table_schema,
still fails, because it outputs a range-key which Scylla added to a view
because of its own implementation needs, but wasn't in the user's
definition of the GSI. I opened a separate issue #5320 for that.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
@nyh
Copy link
Contributor Author

nyh commented Nov 21, 2019

KeySchema, AttributeDefinitions were done in commit 6672059 ("alternator: make DescribeTable return table schema"), but one small corner case remains (in a specific case involving a GSI) and moved to a separate issue #5320.

avikivity pushed a commit that referenced this issue Nov 24, 2019
One of the fields still missing in DescribeTable's response (Refs #5026)
was the table's schema - KeySchema and AttributeDefinitions.

This patch adds this missing feature, and enables the previously-xfailing
test test_describe_table_schema.

A complication of this patch is that in a table with secondary indexes,
we need to return not just the base table's schema, but also the indexes'
schema. The existing tests did not cover that feature, so we add here
two more tests in test_gsi.py for that.

One of these secondary-index schema tests, test_gsi_2_describe_table_schema,
still fails, because it outputs a range-key which Scylla added to a view
because of its own implementation needs, but wasn't in the user's
definition of the GSI. I opened a separate issue #5320 for that.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
@fruch
Copy link
Contributor

fruch commented Feb 7, 2020

@nyh, I've encountered a real example that 6672059 is actually helps with
I've was playing with pynamodb
and it does assume having the KeySchema
https://github.com/pynamodb/PynamoDB/blob/master/pynamodb/connection/base.py#L101

I was using scylla 3.2.0, hence didn't had that. I guess I should use the latest docker for my this specific experiment

@nyh
Copy link
Contributor Author

nyh commented Nov 4, 2020

The ARN in the response was also already fixed, in commit 8c17b5a

This issue is becoming a mess, it's hard to tell what is still missing. I should split it to separate items.

@nyh
Copy link
Contributor Author

nyh commented Nov 4, 2020

Created issue #7550 for ItemCount, #7551 for TableSizeBytes and #11222 for ProvisionedThroughput.
Now, the remaining things to cover in new issues before this issue can be closed are:

  1. BillingModeSummary we return is always the same thing (PAY_PER_REQUEST) regardless of what the user chose when creating the table. This is often good enough, but we should reconsider this.
  2. When there are indexes in a table (GSI or LSI), we only provide partial information for those too - see xfailing test test_gsi_describe (we should probably have a similar test for LSI as well).
  3. The DeleteTable operation returns the return type as DescribeTable, so maybe should return additional values as well.

psarna pushed a commit that referenced this issue Nov 9, 2020
DescribeTable should return a UUID "TableId" in its reponse.
We alread had it for CreateTable, and now this patch adds it to
DescribeTable.

The test for this feature is no longer xfail. Moreover, I improved
the test to not only check that the TableId field is present - it
should also match the documented regular expression (the standard
representation of a UUID).

Refs #5026

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20201104114234.363046-1-nyh@scylladb.com>
@nyh
Copy link
Contributor Author

nyh commented Sep 6, 2022

We have individual issues about all remaining missing fields:

So closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alternator Alternator related Issues user request
Projects
None yet
Development

No branches or pull requests

5 participants