Migrate table service to pipeline architecture #352

ShayMusachanov-dev · 2021-08-27T15:18:22Z

No description provided.

ghost · 2021-08-27T15:18:33Z

All CLA requirements met.

rylev

This is a good start. We'll want to make sure we run this against actual services to make sure things work though. I had a few points that I'd like to be addressed first. I also would like someone more familiar with table storage to take a look as well.

sdk/core/src/errors.rs

sdk/core/src/request.rs

sdk/storage/src/blob/blob/responses/delete_blob_response.rs

sdk/storage/src/table/clients/entity_client.rs

sdk/storage/src/table/clients/table_client.rs

sdk/storage/src/table/operations/list_tables.rs

rylev · 2021-09-01T12:13:54Z

sdk/storage/src/table/table_context.rs

@@ -0,0 +1,4 @@
+#[derive(Debug, Default, Clone, PartialEq, Eq)]
+pub struct TableContext {}


What is this for?

In the creation of PipelineContext, TableContext is the second parameter. At the moment I have no data that needs to be passed between pipeline policies.
Is there a way to create PipelineContext without any Content: C ?

@ShayMusachanov-dev we are actively discussing how to handle pipeline options in #474. The current solution leaves much to be desired as you have noticed here.
I would not fret too much here now as it's bound to change soon.

One particular point I think should be discussed is if we should pass the AuthorizationPolicy down the pipeline instead of embedding it in the policy (as you here like it's currently done in CosmosDB). While the current approach is certainly easier, it prevents the user to change the authorization for a specific call, if need be.

rylev · 2021-09-01T12:15:59Z

sdk/storage/src/table/authorization/authorization_policy.rs

+use crate::{authorization::AuthorizationToken, table_context::TableContext};
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct AuthorizationPolicy {


I'm unfortunately not familiar with authorization for the storage APIs. Can any of this be shared with the authorization implementation in Cosmos?

I've never worked with Cosmos, so I don't want to say anything from basic reading. Of course, if there is a shared code we should use the same implementation

Besides common bits, the auth scheme in Cosmos is a little different: there, for example, you can have "Users" with specific ACLs on documents¹. Here we have SAS tokens².

I think It's best if every crate/function block keeps its own Auth policy.

Footnotes

https://docs.microsoft.com/en-us/rest/api/cosmos-db/create-a-user. ↩

https://docs.microsoft.com/en-us/azure/storage/common/authorize-data-access. ↩

Create example for TableClient, Start implement EntityClient, Remove old implementation, Change the expected status code in create table and entity Implement Insert Entity including Deserialize and Serialize Add 'next_table_name' header value in the response object of ListTable Remove the empty Impl of TableContext

…e review of rylev

…t without implantation.

…zation

cataggar · 2021-10-27T12:11:14Z

@ShayMusachanov-dev, thank you for your work on this. Is this something that we want to merge before azure_storage is published. If so, @ShayMusachanov-dev could you resolve conflicts? @rylev, what do you think?

rylev · 2021-10-28T10:17:00Z

If we resolve conflicts, I think we can try to land this. Thanks for your patience!

ShayMusachanov-dev · 2021-10-29T13:12:45Z

@cataggar, I was thrilled to see your comment. I performed a marge from the Main branch.
@cataggar, @rylev Please let me know if there is any problem or fixes I need to make.

The current implementation does not support the streaming of entities for a given OData query (pagination using continuation token). Is there a generic implementation in the Core crate that I can use or guidelines for similar implementation?

…n DeleteTableResponse

rylev

Just some nits from me. Overall it looks good.

Please note that very large changes like this one are hard to review and land. It would be helpful to try to land smaller changes in the future 😊

sdk/core/src/headers/utilities.rs

sdk/storage/examples/table_client_00.rs

rylev · 2021-10-29T15:43:27Z

sdk/storage/examples/table_client_00.rs

+
+#[tokio::main]
+async fn main() -> Result<(), Error> {
+    // in the follwing example we will interact with the user table. first let's create the table if it doesn't exists;


Suggested change

// in the follwing example we will interact with the user table. first let's create the table if it doesn't exists;

// in the following example we will interact with the user table. first let's create the table if it doesn't exists;

sdk/storage/examples/table_client_00.rs

sdk/storage/src/table/clients/entity_client.rs

sdk/storage/src/table/operations/entity/insert_entity.rs

rylev · 2021-10-29T15:52:57Z

@heaths @MindFlavor if you have the chance to review this as well that would be very helpful! Thank you 🧡

Fix typo and add more console prints to table example. Remove the command of the timeout parameter in the operations Option and append the timeout to the request Uri. Add more documentation on each table and entity operations. Update the prelude file and use import statements correctly.

The execution of the test, causing the workflow to fail. Just as a temporary solution, I put the test in a comment. Until I change the test to use the mock_transport_framework

ShayMusachanov-dev · 2021-10-30T10:18:53Z

@rylev, I have committed changes according to your comments.
Also, I put the test of table_client in a comment. I'll start using the mock_transport_framework as soon as possible.

MindFlavor

Excellent work, thank you!
I love it ❤️!

I've tested it against both Azure and the emulator and it works.

There are some minors changes that I suggest, but after these I think this is good to go.

MindFlavor · 2021-11-03T09:11:49Z

sdk/core/src/request.rs

@@ -112,6 +118,16 @@ impl Request {
    pub fn clone_body(&self) -> Body {
        self.body.clone()
    }
+
+    /// Get a mutable reference to the request's body.


I think this is not needed. You can use set_body instead.

MindFlavor · 2021-11-03T09:12:49Z

sdk/storage/src/table/operations/table/create_table.rs

+        let md5 = base64::encode(&md5::compute(bytes.as_ref())[..]);
+        headers.append("Content-MD5", HeaderValue::from_str(md5.as_str()).unwrap());
+
+        *request.body_mut() = bytes.into();


Suggested change

*request.body_mut() = bytes.into();

request.set_body(bytes.into());

MindFlavor · 2021-11-03T09:15:27Z

sdk/storage/examples/table_client_00.rs

+    ];
+
+    let table_name = "users";
+    let table_client = TableClient::emulator(TableOptions::default());


I'd rather give the ability to test against Azure as well. In the SDK we have the unwritten convention to pass storage account and master key with env variables.

Something like:

let account = std::env::vars() .find(|i| i.0 == "STORAGE_ACCOUNT") .map(|i| i.1); let key = std::env::vars() .find(|i| i.0 == "STORAGE_MASTER_KEY") .map(|i| i.1); let auth_token = match (account.as_ref(), key) { (Some(account), Some(key)) => Some(AuthorizationToken::SharedKeyToken { account: account.to_owned(), key, }), _ => None, }; let table_name = "users"; let table_client = auth_token .as_ref() .map(|auth_token| { TableClient::new( account.clone().unwrap(), auth_token.clone(), TableOptions::default(), ) }) .unwrap_or_else(|| TableClient::emulator(TableOptions::default()));

MindFlavor · 2021-11-03T09:20:03Z

sdk/storage/src/table/clients/entity_client.rs

-    partition_key_client: Arc<PartitionKeyClient>,
-    row_key: String,
-    url: Url,
+    table_name: Cow<'static, str>,


I would prefer a simpler String. Table names should be relatively small anyway.

MindFlavor · 2021-11-03T09:20:44Z

sdk/storage/src/table/clients/entity_client.rs

-        self.partition_key_client.http_client()
+    /// Creates a new EntityClient for the given table_name.
+    /// Consuming table_client in the process.
+    pub fn new<NAME: Into<Cow<'static, str>>>(table_client: TableClient, table_name: NAME) -> Self {


Suggested change

pub fn new<NAME: Into<Cow<'static, str>>>(table_client: TableClient, table_name: NAME) -> Self {

pub fn new(table_client: TableClient, table_name: impl Into<String>) -> Self {

MindFlavor · 2021-11-03T09:21:32Z

sdk/storage/src/table/clients/table_client.rs

-    async fn test_insert() {
-        let table_client = get_emulator_client();
+    /// Crates Entity client for a given table. consuming Self in the process.
+    pub fn into_entity_client<S: Into<Cow<'static, str>>>(self, table_name: S) -> EntityClient {


Suggested change

pub fn into_entity_client<S: Into<Cow<'static, str>>>(self, table_name: S) -> EntityClient {

pub fn into_entity_client(self, table_name: impl Into<String>) -> EntityClient {

Try to read the storage account name and key from the environment, else create an emulator client. Use the set_body fn instead of body_mut. Replace Cow smart pointer with From<String> in table and entity clients.

MindFlavor

Thank you very much! 👍

…itecture

…ine-architecture" This reverts commit f8ea176, reversing changes made to 64ae682.

ShayMusachanov-dev added 3 commits August 27, 2021 13:49

implement Azure table client 'List Table' operation to use pipeline

f3837be

remove connection string

a3d21e9

implement delete and create table operations

fe9301e

ShayMusachanov-dev added 2 commits August 27, 2021 19:57

add Top and Filter to 'ListTableOptions'

8d77b0f

create entity client

ecda806

rylev suggested changes Sep 1, 2021

View reviewed changes

rylev requested a review from heaths September 1, 2021 12:17

ShayMusachanov-dev added 7 commits September 3, 2021 00:46

remove the old table client code

7d53096

continue in EntityClient implementation. Changes according to the cod…

e625da4

…e review of rylev

add more doc to entity client, add missing operations to entity clien…

2db1afa

…t without implantation.

fix delete entity headers building, implement update entity

3b6e3fb

add more documentation, use one struct for entity and table deseriali…

3199b19

…zation

undo

e8cb99e

ShayMusachanov-dev requested a review from rylev September 13, 2021 17:33

cataggar added the Storage Storage Service (Queues, Blobs, Files) label Oct 26, 2021

cataggar added this to the azure_storage 0.1.0 milestone Oct 26, 2021

ShayMusachanov-dev added 3 commits October 29, 2021 14:40

rename Entities to Entity

a81a771

Merge branch 'main' into migrate-table-service-to-pipeline-architecture

b25b451

fix use statements.

645cb71

ShayMusachanov-dev added 6 commits October 29, 2021 16:20

fix fmt and remove unused import

b03af44

ignore account field

cf03361

comment out timeout option in tableOptions

fcba53c

remove table client test containing println

8ee565c

add derive Default, Debug, Clone, PartialEq, Serialize, Deserialize o…

4533671

…n DeleteTableResponse

fix delete table return type

e52b983

fix table_client00 example

7ca49c1

rylev reviewed Oct 29, 2021

View reviewed changes

rylev requested a review from MindFlavor October 29, 2021 15:52

ShayMusachanov-dev added 2 commits October 30, 2021 12:01

put the test in a comment

e529a8f

The execution of the test, causing the workflow to fail. Just as a temporary solution, I put the test in a comment. Until I change the test to use the mock_transport_framework

start using the mock_transport_framework to test TableClient

1fa8e71

cataggar modified the milestones: azure_storage_datalake 0.1.0, azure_data_tables 0.1.0 Nov 3, 2021

MindFlavor suggested changes Nov 3, 2021

View reviewed changes

MindFlavor added Client This issue points to a problem in the data-plane of the library. storage_tables labels Nov 3, 2021

Changes according to @MindFlavor review.

41aa8b5

Try to read the storage account name and key from the environment, else create an emulator client. Use the set_body fn instead of body_mut. Replace Cow smart pointer with From<String> in table and entity clients.

MindFlavor approved these changes Nov 4, 2021

View reviewed changes

ShayMusachanov-dev and others added 7 commits November 5, 2021 15:00

applying suggestions of clippy

64ae682

Merge branch 'Azure:main' into migrate-table-service-to-pipeline-arch…

f8ea176

…itecture

Revert "Merge branch 'Azure:main' into migrate-table-service-to-pipel…

9855f92

…ine-architecture" This reverts commit f8ea176, reversing changes made to 64ae682.

add req into the trace log

0c2b425

implement sas for azure table strage

af88d79

continue the implementation of table storage sas

42f500c

add test for table sas creation and improve error handling

7596323

ShayMusachanov-dev closed this Dec 9, 2021

ShayMusachanov-dev deleted the migrate-table-service-to-pipeline-architecture branch December 9, 2021 16:08

ShayMusachanov-dev restored the migrate-table-service-to-pipeline-architecture branch December 9, 2021 16:09

ShayMusachanov-dev deleted the migrate-table-service-to-pipeline-architecture branch December 9, 2021 16:11

ShayMusachanov-dev restored the migrate-table-service-to-pipeline-architecture branch December 10, 2021 13:59

ShayMusachanov-dev deleted the migrate-table-service-to-pipeline-architecture branch December 10, 2021 14:00

ShayMusachanov-dev restored the migrate-table-service-to-pipeline-architecture branch December 10, 2021 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate table service to pipeline architecture #352

Migrate table service to pipeline architecture #352

ShayMusachanov-dev commented Aug 27, 2021

ghost commented Aug 27, 2021 •

edited by ghost

Loading

rylev left a comment

rylev Sep 1, 2021

ShayMusachanov-dev Sep 7, 2021 •

edited

Loading

MindFlavor Nov 3, 2021

rylev Sep 1, 2021

ShayMusachanov-dev Sep 6, 2021

MindFlavor Nov 3, 2021

cataggar commented Oct 27, 2021

rylev commented Oct 28, 2021

ShayMusachanov-dev commented Oct 29, 2021

rylev left a comment

rylev Oct 29, 2021

rylev commented Oct 29, 2021

ShayMusachanov-dev commented Oct 30, 2021

MindFlavor left a comment

MindFlavor Nov 3, 2021

MindFlavor Nov 3, 2021

MindFlavor Nov 3, 2021

MindFlavor Nov 3, 2021

MindFlavor Nov 3, 2021

MindFlavor Nov 3, 2021

MindFlavor left a comment

		@@ -0,0 +1,4 @@
		#[derive(Debug, Default, Clone, PartialEq, Eq)]
		pub struct TableContext {}

	// in the follwing example we will interact with the user table. first let's create the table if it doesn't exists;
	// in the following example we will interact with the user table. first let's create the table if it doesn't exists;

	*request.body_mut() = bytes.into();
	request.set_body(bytes.into());

	pub fn new<NAME: Into<Cow<'static, str>>>(table_client: TableClient, table_name: NAME) -> Self {
	pub fn new(table_client: TableClient, table_name: impl Into<String>) -> Self {

	pub fn into_entity_client<S: Into<Cow<'static, str>>>(self, table_name: S) -> EntityClient {
	pub fn into_entity_client(self, table_name: impl Into<String>) -> EntityClient {

Migrate table service to pipeline architecture #352

Migrate table service to pipeline architecture #352

Conversation

ShayMusachanov-dev commented Aug 27, 2021

ghost commented Aug 27, 2021 • edited by ghost Loading

rylev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShayMusachanov-dev Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

cataggar commented Oct 27, 2021

rylev commented Oct 28, 2021

ShayMusachanov-dev commented Oct 29, 2021

rylev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylev commented Oct 29, 2021

ShayMusachanov-dev commented Oct 30, 2021

MindFlavor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MindFlavor left a comment

Choose a reason for hiding this comment

ghost commented Aug 27, 2021 •

edited by ghost

Loading

ShayMusachanov-dev Sep 7, 2021 •

edited

Loading