-
Notifications
You must be signed in to change notification settings - Fork 50
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
Multiple Import Metadata actions #1178
base: mvp_demo
Are you sure you want to change the base?
Multiple Import Metadata actions #1178
Conversation
@msvinaykumar can you please review the PR |
@shreyabiradar07 can you start MD file describe above functionality |
@@ -94,4 +94,6 @@ public interface ExperimentDAO { | |||
// Load metadata | |||
List<KruizeDSMetadataEntry> loadMetadata() throws Exception; | |||
|
|||
// Delete metadata | |||
public ValidationOutputData deleteKruizeDSMetadataEntryByName(String dataSourceName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could name it something like "removeClusterMetadataByName" or
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have followed the similar naming convention as deleteKruizeExperimentEntryByName
, please confirm if this still requires renaming.
@@ -94,4 +94,6 @@ public interface ExperimentDAO { | |||
// Load metadata | |||
List<KruizeDSMetadataEntry> loadMetadata() throws Exception; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadClusterMetaData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently there no usages of loadMetadata
function as it fetches all the records from KruizeDSMetadataEntry
table (in case when there are multiple datasources will retrieve all the container data not limiting to a specific cluster), hence will remove the unused functionality.
Documented delete metadata functionality in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
UI Flow --> First get if no data then POST and get if still no data then give up (Display error if any) curl -X post http://${URL}/dsmetadata -d @./input_datasource.json For above request in backend curl -X post http://${URL}/dsmetadata -d @./input_datasource.json For above request in backend curl -X get http://${URL}/dsmetadata?datasource_name="abc" UserWorflow
No need of refresh flag |
{ |
@shreyabiradar07 please set up code walk through session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
} | ||
|
||
new DataSourceManager().importMetadataFromDataSource(datasource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the review, this can cause DB corruption if for any reason the import fails as we would have removed the DB content. Please reverse the order and add validation to the import results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code flow including validation of imported DataSourceMetadataInfo
object
|
||
inputData = request.getReader().lines().collect(Collectors.joining()); | ||
if (null == inputData || inputData.isEmpty()) { | ||
throw new Exception("Request input data cannot be null or empty"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all of the strings to string constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
DataSourceMetadataInfo dataSourceMetadata = new ExperimentDBService().loadMetadataFromDBByName(dataSourceName, "false"); | ||
if (null != dataSourceMetadata) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all of the code that handles the doPost to the relevant files under datasource
@@ -110,6 +110,10 @@ public void deleteMetadataFromDataSource(DataSourceInfo dataSource) { | |||
*/ | |||
public void saveMetadataFromDataSourceToDB(DataSourceInfo dataSourceInfo) { | |||
try { | |||
if (null == dataSourceInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shreyabiradar07 - As discussed, please add the negative scenario to bring down the prometheus by scaling down the statefulset and deployment between multiple imports, and validate that the second call to import metadata fails with appropriate error message and list metadata returns the metadata from the DB. This can be added in a separate PR.
This PR has following changes:
curl -X DELETE http://${URL}/dsmetadata -d @./input_datasource.json
Example
input_datasource.json
(same as that of import dsmetadata request body)