Skip to content

Commit

Permalink
Fix setTag logic to prevent high cpu load on rds (#572)
Browse files Browse the repository at this point in the history 
* remove unused code + use insert if not exist for values_string instead of loading from memory + test

* address comments

---------

Co-authored-by: Yingjian Wu <[email protected]>
  • Loading branch information
@stevie9868
stevie9868 and Yingjian Wu authored Mar 8, 2024
1 parent 6d7bfca commit e8ea9f9
Show file tree
Hide file tree
Showing 5 changed files with 187 additions and 89 deletions.
18 changes: 7 additions & 11 deletions ...on-server/src/main/java/com/netflix/metacat/common/server/usermetadata/LookupService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,14 @@
public interface LookupService {
/**
* Returns the lookup for the given <code>name</code>.
* If includeValues = true, we will set the Lookup values with the associated name, otherwise empty set
*
* @param name lookup name
* @param includeValues whether we should set the values or not in the Lookup Object
* @return lookup
*/
@Nullable
default Lookup get(final String name) {
default Lookup get(final String name, final boolean includeValues) {
return null;
}

Expand Down Expand Up @@ -75,22 +77,20 @@ default Set<String> getValues(final Long lookupId) {
*
* @param name lookup name
* @param values multiple values
* @param includeValues whether to populate the values field in the Lookup Object
* @return updated lookup
*/
@Nullable
default Lookup setValues(final String name, final Set<String> values) {
return null;
}

/**
* Saves the lookup value.
*
* @param name lookup name
* @param values multiple values
* @param includeValues whether to include values in the final Lookup Object
* @return updated lookup
*/
@Nullable
default Lookup addValues(final String name, final Set<String> values) {
default Lookup addValues(final String name, final Set<String> values, boolean includeValues) {
return null;
}

Expand All @@ -99,11 +99,7 @@ default Lookup addValues(final String name, final Set<String> values) {
*
* @param name lookup name
* @param value lookup value
* @param includeValues whether to return lookup value in the Lookup Object
* @return updated lookup
*/
@Nullable
default Lookup setValue(final String name, final String value) {
return null;
}

}
79 changes: 79 additions & 0 deletions ...at-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MetacatSmokeSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1771,4 +1771,83 @@ class MetacatSmokeSpec extends Specification {
'embedded-hive-metastore/invalid/dm/vm',
'embedded-hive-metastore/invalid/dm/vm=1']
}
def "Test Set tags for all scenarios"() {
setup:
// The setup already contains the following 7 tags
// [data-category:non-customer, test, test_tag, unused, do_not_drop, iceberg_migration_do_not_modify, do_not_rename]
def catalogName = "hive-metastore"
def databaseName = "default"
def catalog = QualifiedName.ofCatalog(catalogName)
def database = QualifiedName.ofDatabase(catalogName, databaseName)
def table1Name = "table1"
def table1Tags = ['table1', 'mock'] as Set
def table2Name = "table2"
def table2Tags = ['table2', 'mock'] as Set
def table3Name = "table3"
def table3Tags = ['table3', 'bdow'] as Set
when:
// add 2 new tags on table1
createTable(catalogName, databaseName, table1Name)
tagApi.setTableTags(catalogName, databaseName, table1Name, table1Tags)
then:
tagApi.getTags().size() == 9
tagApi.list(['table1', 'mock'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 1
tagApi.search('mock', null, null, null).size() == 1
tagApi.search('table1', null, null, null).size() == 1
metacatJson.convertValue(api.getTable(catalogName, databaseName, table1Name, false, true, true).getDefinitionMetadata().get('tags'), Set.class) == table1Tags
when:
// add an existing tag mock but a new tag on table 2
createTable(catalogName, databaseName, table2Name)
tagApi.setTableTags(catalogName, databaseName, table2Name, table2Tags)
then:
tagApi.getTags().size() == 10
tagApi.list(['table2'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 1
tagApi.list(['mock'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 2
tagApi.list(['table1', 'mock'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 2
tagApi.search('mock', null, null, null).size() == 2
tagApi.search('table1', null, null, null).size() == 1
tagApi.search('table2', null, null, null).size() == 1
metacatJson.convertValue(api.getTable(catalogName, databaseName, table2Name, false, true, true).getDefinitionMetadata().get('tags'), Set.class) == table2Tags
when:
// delete table1, for now even if tag table1 is only applied to table1, we will not remove the tag from our db
api.deleteTable(catalogName, databaseName, "table1")
then:
tagApi.getTags().size() == 10
tagApi.list(['table1'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 1
tagApi.list(['table2', 'mock'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 2
tagApi.list(['mock'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 2
tagApi.search('mock', null, null, null).size() == 2
tagApi.search('table1', null, null, null).size() == 1
tagApi.search('table2', null, null, null).size() == 1
when:
// add tags on catalog database and table level through setTags api
// create 1 new catalogTag on catalog
// create 1 new databaseTag on database
// create 2 new tableTags on table3
tagApi.setTags(TagCreateRequestDto.builder().name(catalog).tags(['mock', 'catalogTag'] as List<String>).build())
tagApi.setTags(TagCreateRequestDto.builder().name(database).tags(['mock', 'databaseTag'] as List<String>).build())
createTable(catalogName, databaseName, "table3")
def table3 = QualifiedName.ofTable(catalogName, databaseName, table3Name)
tagApi.setTags(TagCreateRequestDto.builder().name(table3).tags(table3Tags as List<String>).build())
then:
tagApi.getTags().size() == 14
tagApi.list(['table2', 'mock'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 2
tagApi.list(['table3', 'bdow'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 1
tagApi.list(['mock'] as Set<String>, null, null, null, null, QualifiedName.Type.TABLE).size() == 2
tagApi.list(['mock'] as Set<String>, null, null, null, null, QualifiedName.Type.DATABASE).size() == 1
tagApi.list(['mock'] as Set<String>, null, null, null, null, QualifiedName.Type.CATALOG).size() == 1
tagApi.list(['mock'] as Set<String>, null, null, null, null, null).size() == 4
tagApi.search('mock', null, null, null).size() == 4
tagApi.search('table3', null, null, null).size() == 1
tagApi.search('bdow', null, null, null).size() == 1
tagApi.search('table2', null, null, null).size() == 1
metacatJson.convertValue(api.getTable(catalogName, databaseName, table3Name, false, true, true).getDefinitionMetadata().get('tags'), Set.class) == table3Tags
cleanup:
api.deleteTable(catalogName, databaseName, "table2")
api.deleteTable(catalogName, databaseName, "table3")
}
}
93 changes: 18 additions & 75 deletions ...t-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlLookupService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

package com.netflix.metacat.metadata.mysql;

import com.google.common.base.Joiner;
import com.google.common.collect.Sets;
import com.netflix.metacat.common.server.model.Lookup;
import com.netflix.metacat.common.server.properties.Config;
Expand All @@ -23,7 +22,6 @@
import lombok.extern.slf4j.Slf4j;
import org.springframework.dao.EmptyResultDataAccessException;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.SqlParameterValue;
import org.springframework.jdbc.support.GeneratedKeyHolder;
import org.springframework.jdbc.support.KeyHolder;
import org.springframework.transaction.annotation.Transactional;
Expand All @@ -32,6 +30,7 @@
import java.sql.SQLException;
import java.sql.Statement;
import java.sql.Types;
import java.util.Collections;
import java.util.Set;
import java.util.stream.Collectors;

Expand All @@ -50,8 +49,9 @@ public class MySqlLookupService implements LookupService {
+ " values (?,0,?,?,?,now(),now())";
private static final String SQL_INSERT_LOOKUP_VALUES =
"insert into lookup_values( lookup_id, values_string) values (?,?)";
private static final String SQL_DELETE_LOOKUP_VALUES =
"delete from lookup_values where lookup_id=? and values_string in (%s)";
private static final String SQL_INSERT_LOOKUP_VALUE_IF_NOT_EXIST =
"INSERT IGNORE INTO lookup_values (lookup_id, values_string) VALUES (?, ?)";

private static final String SQL_GET_LOOKUP_VALUES =
"select values_string value from lookup_values where lookup_id=?";
private static final String SQL_GET_LOOKUP_VALUES_BY_NAME =
Expand Down Expand Up @@ -79,7 +79,7 @@ public MySqlLookupService(final Config config, final JdbcTemplate jdbcTemplate)
*/
@Override
@Transactional(readOnly = true)
public Lookup get(final String name) {
public Lookup get(final String name, final boolean includeValues) {
try {
return jdbcTemplate.queryForObject(
SQL_GET_LOOKUP,
Expand All @@ -93,7 +93,7 @@ public Lookup get(final String name) {
lookup.setLastUpdated(rs.getDate("lastUpdated"));
lookup.setLastUpdatedBy(rs.getString("lastUpdatedBy"));
lookup.setDateCreated(rs.getDate("dateCreated"));
lookup.setValues(getValues(rs.getLong("id")));
lookup.setValues(includeValues ? getValues(rs.getLong("id")) : Collections.EMPTY_SET);
return lookup;
});
} catch (EmptyResultDataAccessException e) {
Expand Down Expand Up @@ -162,61 +162,22 @@ public Set<String> getValues(final String name) {
}
}

/**
* Saves the lookup value.
*
* @param name lookup name
* @param values multiple values
* @return returns the lookup with the given name.
*/
@Override
public Lookup setValues(final String name, final Set<String> values) {
try {
final Lookup lookup = findOrCreateLookupByName(name);
final Set<String> inserts;
Set<String> deletes = Sets.newHashSet();
final Set<String> lookupValues = lookup.getValues();
if (lookupValues == null || lookupValues.isEmpty()) {
inserts = values;
} else {
inserts = Sets.difference(values, lookupValues).immutableCopy();
deletes = Sets.difference(lookupValues, values).immutableCopy();
}
lookup.setValues(values);
if (!inserts.isEmpty()) {
insertLookupValues(lookup.getId(), inserts);
}
if (!deletes.isEmpty()) {
deleteLookupValues(lookup.getId(), deletes);
}
return lookup;
} catch (Exception e) {
final String message = String.format("Failed to set the lookup values for name %s", name);
log.error(message, e);
throw new UserMetadataServiceException(message, e);
}
}

private void insertLookupValues(final Long id, final Set<String> inserts) {
jdbcTemplate.batchUpdate(SQL_INSERT_LOOKUP_VALUES, inserts.stream().map(insert -> new Object[]{id, insert})
private void insertLookupValuesIfNotExist(final Long id, final Set<String> inserts) {
jdbcTemplate.batchUpdate(SQL_INSERT_LOOKUP_VALUE_IF_NOT_EXIST,
inserts.stream().map(insert -> new Object[]{id, insert})
.collect(Collectors.toList()), new int[]{Types.BIGINT, Types.VARCHAR});
}

private void deleteLookupValues(final Long id, final Set<String> deletes) {
jdbcTemplate.update(
String.format(SQL_DELETE_LOOKUP_VALUES, "'" + Joiner.on("','").skipNulls().join(deletes) + "'"),
new SqlParameterValue(Types.BIGINT, id));
}

/**
* findOrCreateLookupByName.
*
* @param name name to find or create
* @param includeValues whether to include the values in the Lookup Object
* @return Look up object
* @throws SQLException sql exception
*/
private Lookup findOrCreateLookupByName(final String name) throws SQLException {
Lookup lookup = get(name);
private Lookup findOrCreateLookupByName(final String name, final boolean includeValues) throws SQLException {
Lookup lookup = get(name, includeValues);
if (lookup == null) {
final KeyHolder holder = new GeneratedKeyHolder();
jdbcTemplate.update(connection -> {
Expand Down Expand Up @@ -244,20 +205,14 @@ private Lookup findOrCreateLookupByName(final String name) throws SQLException {
* @return returns the lookup with the given name.
*/
@Override
public Lookup addValues(final String name, final Set<String> values) {
public Lookup addValues(final String name, final Set<String> values, final boolean includeValues) {
try {
final Lookup lookup = findOrCreateLookupByName(name);

final Set<String> inserts;
final Set<String> lookupValues = lookup.getValues();
if (lookupValues == null || lookupValues.isEmpty()) {
inserts = values;
lookup.setValues(values);
} else {
inserts = Sets.difference(values, lookupValues);
final Lookup lookup = findOrCreateLookupByName(name, includeValues);
if (!values.isEmpty()) {
insertLookupValuesIfNotExist(lookup.getId(), values);
}
if (!inserts.isEmpty()) {
insertLookupValues(lookup.getId(), inserts);
if (includeValues) {
lookup.getValues().addAll(values);
}
return lookup;
} catch (Exception e) {
Expand All @@ -266,16 +221,4 @@ public Lookup addValues(final String name, final Set<String> values) {
throw new UserMetadataServiceException(message, e);
}
}

/**
* Saves the lookup value.
*
* @param name lookup name
* @param value lookup value
* @return returns the lookup with the given name.
*/
@Override
public Lookup setValue(final String name, final String value) {
return setValues(name, Sets.newHashSet(value));
}
}
6 changes: 3 additions & 3 deletions metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlTagService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,9 @@ public MySqlTagService(
this.userMetadataService = Preconditions.checkNotNull(userMetadataService, "userMetadataService is required");
}

private Lookup addTags(final Set<String> tags) {
private Lookup addTags(final Set<String> tags, final boolean includeValues) {
try {
return lookupService.addValues(LOOKUP_NAME_TAG, tags);
return lookupService.addValues(LOOKUP_NAME_TAG, tags, includeValues);
} catch (Exception e) {
final String message = String.format("Failed adding the tags %s", tags);
log.error(message, e);
Expand Down Expand Up @@ -396,7 +396,7 @@ public List<QualifiedName> search(
@Override
public Set<String> setTags(final QualifiedName name, final Set<String> tags,
final boolean updateUserMetadata) {
addTags(tags);
addTags(tags, false);
try {
final TagItem tagItem = findOrCreateTagItemByName(name.toString());
final Set<String> inserts;
Expand Down
Loading

0 comments on commit e8ea9f9

Please sign in to comment.