Yingjianw/additional safety rollout #602

stevie9868 · 2024-07-04T21:34:54Z

First commit:

Add a flag to controll the rollout in case there are any performance degradation or bug for each api call (create, rename, drop, get) that can touch the parent child relation service. This flag can also be used in the long run if we see performance degradation as we have more parent child relationship created and we can quickly block certain actions.
Add a validation to make sure the parent name specified in the request actually exists in metacat
One bug fix on the rename side even though we are going to rewrite to use two phase commit idea in this pr improve rename logic to use 2 phase commit idea #600, which already handled this case. I have also added this case in our e2e test

More specifically the bug is:

Supposed that we have childName = 'c' and parentName = 'p', and let's ignore uuid and clone type for example.
Then we insert a new parent child relationship childName = 'nc' and parentName = 'p'
So for now we have two records:
1. 'c' | 'p'
1. 'nc' | 'p'
Then we rename 'c' to 'nc' because we always rename in parent child relationship first, and after the rename, the db will look something like
1. 'nc' | 'p'
1. 'nc' | 'p'
But it will fail later in the tableProxy as table 'nc' already and then if we revert back it will make all 2 records as 'c' | 'p'

Second commit

Add a validation during createTable to prevent creating a new record having the same name in the existing mysql db but uuid is different from that of the exiting one.

Third commit

Add a search endpoint for getParentsApi in case we need to block the get api and we can still have way to debug

Forth commit

Move out other parent child config out to keep all validation logic in the same place and also the config is general enough and doesn't need to be netflix specific

Fifth commit
Make all the flags to false by default

amboz · 2024-07-08T22:02:39Z

metacat-client/src/main/java/com/netflix/metacat/client/api/ParentChildRelV1.java

+ @Path("parents/catalog/{catalog-name}/database/{database-name}/table/{table-name}")
+ @Consumes(MediaType.APPLICATION_JSON)
+ @Produces(MediaType.APPLICATION_JSON)
+ Set<ParentInfoDto> getParents(


one the one hand, i would suggest we rename this to getParent since we are enforcing the one-parent-per-child rule and its a little confusing as the name implies we can expect more than one results. on the other hand, this API does technically return more than one result, so we would need to have a different return type to make this change. i have a weak preference to rename it and change the contract despite it becoming less flexible, but its up to you

If the api stays having multiple return values then plural is better so that the correspondence holds, otherwise singular. There is no current use case for multiple parents but keeping the flexibility is reasonable.

I was thinking the same thing before the implementation but ultimately choose to have the api flexibile that can return multiple parents.

Another benefit is if indeed somehow we end up with two parents on the same child table due to db isolation issues, we can still get them instead of just one.

amboz · 2024-07-08T22:06:16Z

metacat-main/src/main/java/com/netflix/metacat/main/api/v1/ParentChildRelController.java

@@ -72,4 +73,35 @@ public Set<ChildInfoDto> getChildren(
 )
 );
 }
+
+ /**
+ * Return the list of children for a given table.


Suggested change

* Return the list of children for a given table.

* Return the list of parent info for a given table.

amboz · 2024-07-08T22:06:30Z

metacat-main/src/main/java/com/netflix/metacat/main/api/v1/ParentChildRelController.java

+ * @param catalogName catalogName
+ * @param databaseName databaseName
+ * @param tableName tableName
+ * @return list of childInfoDto


Suggested change

* @return list of childInfoDto

* @return list of parentInfoDto

amboz · 2024-07-08T22:09:36Z

...sql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlParentChildRelMetaDataService.java

+ maxAllow = props.getMaxAllow();
+ }
+
+ // if maxAllow < 0, this means we can create as many child table under the parent


Suggested change

// if maxAllow < 0, this means we can create as many child table under the parent

// if maxAllow < 0, this means we can create unlimited child tables under the parent

insyncoss · 2024-07-09T08:29:00Z

metacat-main/src/main/java/com/netflix/metacat/main/services/impl/TableServiceImpl.java

+ throw new RuntimeException(name + " is a child table and "
+ + "dropping a child table is currently disabled");
+ }
+ if (parentChildRelMetadataService.isParentTable(name)) {


Regardless of this check the dropping of parent table should be disabled anyways right

insyncoss · 2024-07-09T08:35:36Z

...sql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlParentChildRelMetaDataService.java

+ final String type,
+ final ParentChildRelationshipProperties props) {
+ // Validate max clone allow
+ // First check if the parent table have configured max allowed on the table config


Suggested change

// First check if the parent table have configured max allowed on the table config

// First check if the parent table has a configured max allowed setting on the table config

insyncoss · 2024-07-09T08:35:58Z

...sql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlParentChildRelMetaDataService.java

+ true
+ );
+
+ // Then check if the parent have configured max allowed on the db config


Suggested change

// Then check if the parent have configured max allowed on the db config

// Then check if the parent has a configured max allowed setting on the db config

insyncoss · 2024-07-09T08:36:22Z

...sql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlParentChildRelMetaDataService.java

+ );
+ }
+
+ // If not specified in maxAllowPerDBPerRelType,check the default max Allow based on relationType


Suggested change

// If not specified in maxAllowPerDBPerRelType,check the default max Allow based on relationType

// If not specified in maxAllowPerDBPerRelType, check the default maxAllow based on relationType

insyncoss · 2024-07-09T08:38:21Z

metacat-client/src/main/java/com/netflix/metacat/client/api/ParentChildRelV1.java

+ @Path("parents/catalog/{catalog-name}/database/{database-name}/table/{table-name}")
+ @Consumes(MediaType.APPLICATION_JSON)
+ @Produces(MediaType.APPLICATION_JSON)
+ Set<ParentInfoDto> getParents(


If the api stays having multiple return values then plural is better so that the correspondence holds, otherwise singular. There is no current use case for multiple parents but keeping the flexibility is reasonable.

stevie9868 · 2024-07-09T12:34:30Z

Thanks all, I will address the comments in the next PR.

safety flag to control the parent child relationship rollout

8317b70

stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch from 22761f7 to 8317b70 Compare July 4, 2024 23:06

Add validaton to prevent creating another new name with a different uuid

c91c57e

stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch from 807fc52 to c91c57e Compare July 5, 2024 02:22

add search endpoint for getParents

084e314

stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch 3 times, most recently from f0a6bab to 300947a Compare July 6, 2024 22:25

moveConfig out to oss

9bd01c5

stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch 4 times, most recently from 5ef08bb to 13fe8a7 Compare July 7, 2024 14:48

default flags to false

17ad8a2

stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch from 13fe8a7 to 17ad8a2 Compare July 7, 2024 14:57

use limit 1 to speed up check exist query

133d10a

amboz approved these changes Jul 8, 2024

View reviewed changes

insyncoss approved these changes Jul 9, 2024

View reviewed changes

stevie9868 merged commit b585cda into master Jul 9, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yingjianw/additional safety rollout #602

Yingjianw/additional safety rollout #602

stevie9868 commented Jul 4, 2024 •

edited

Loading

amboz Jul 8, 2024

insyncoss Jul 9, 2024

stevie9868 Jul 9, 2024 •

edited

Loading

amboz Jul 8, 2024

amboz Jul 8, 2024

amboz Jul 8, 2024

insyncoss Jul 9, 2024

insyncoss Jul 9, 2024

insyncoss Jul 9, 2024

insyncoss Jul 9, 2024

insyncoss Jul 9, 2024

stevie9868 commented Jul 9, 2024

	* Return the list of children for a given table.
	* Return the list of parent info for a given table.

	* @return list of childInfoDto
	* @return list of parentInfoDto

	// if maxAllow < 0, this means we can create as many child table under the parent
	// if maxAllow < 0, this means we can create unlimited child tables under the parent

	// First check if the parent table have configured max allowed on the table config
	// First check if the parent table has a configured max allowed setting on the table config

	// Then check if the parent have configured max allowed on the db config
	// Then check if the parent has a configured max allowed setting on the db config

	// If not specified in maxAllowPerDBPerRelType,check the default max Allow based on relationType
	// If not specified in maxAllowPerDBPerRelType, check the default maxAllow based on relationType

Yingjianw/additional safety rollout #602

Yingjianw/additional safety rollout #602

Conversation

stevie9868 commented Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevie9868 Jul 9, 2024 • 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

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

stevie9868 commented Jul 9, 2024

stevie9868 commented Jul 4, 2024 •

edited

Loading

stevie9868 Jul 9, 2024 •

edited

Loading