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

Yingjianw/additional safety rollout #602

Merged
merged 6 commits into from
Jul 9, 2024

Conversation

stevie9868
Copy link
Contributor

@stevie9868 stevie9868 commented Jul 4, 2024

First commit:

  1. 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.
  2. Add a validation to make sure the parent name specified in the request actually exists in metacat
  3. 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

  1. 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

  1. 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

  1. 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

@stevie9868 stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch from 22761f7 to 8317b70 Compare July 4, 2024 23:06
@stevie9868 stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch from 807fc52 to c91c57e Compare July 5, 2024 02:22
@stevie9868 stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch 3 times, most recently from f0a6bab to 300947a Compare July 6, 2024 22:25
@stevie9868 stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch 4 times, most recently from 5ef08bb to 13fe8a7 Compare July 7, 2024 14:48
@stevie9868 stevie9868 force-pushed the yingjianw/additionalSafetyRollout branch from 13fe8a7 to 17ad8a2 Compare July 7, 2024 14:57
@Path("parents/catalog/{catalog-name}/database/{database-name}/table/{table-name}")
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
Set<ParentInfoDto> getParents(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@stevie9868 stevie9868 Jul 9, 2024

Choose a reason for hiding this comment

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

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.

@@ -72,4 +73,35 @@ public Set<ChildInfoDto> getChildren(
)
);
}

/**
* Return the list of children for a given table.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* @param catalogName catalogName
* @param databaseName databaseName
* @param tableName tableName
* @return list of childInfoDto
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @return list of childInfoDto
* @return list of parentInfoDto

maxAllow = props.getMaxAllow();
}

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

true
);

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

Choose a reason for hiding this comment

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

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

);
}

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

@stevie9868 stevie9868 merged commit b585cda into master Jul 9, 2024
2 checks passed
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.

3 participants