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.

Sign up for GitHub

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

Add Table metadata cache expiration configuration #321

Merged
merged 2 commits into from
Oct 1, 2021
Merged

Add Table metadata cache expiration configuration #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79636bf
Add Table metadata cache expiration configuration
brfrn169 Sep 29, 2021
4d30bab
Merge branch 'master' into add-table-metadata-cache-expiration-config…
brfrn169 Oct 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions core/src/main/java/com/scalar/db/config/DatabaseConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ public class DatabaseConfig {
private Class<? extends DistributedTransactionManager> transactionManagerClass;
private Class<? extends TwoPhaseCommitTransactionManager> twoPhaseCommitTransactionManagerClass;
private Isolation isolation = Isolation.SNAPSHOT;
private long tableMetadataCacheExpirationTimeSecs = -1;

public static final String PREFIX = "scalar.db.";
public static final String CONTACT_POINTS = PREFIX + "contact_points";
public static final String CONTACT_PORT = PREFIX + "contact_port";
Expand All @@ -60,6 +62,8 @@ public class DatabaseConfig {
public static final String NAMESPACE_PREFIX = PREFIX + "namespace_prefix";
public static final String TRANSACTION_MANAGER = PREFIX + "transaction_manager";
public static final String ISOLATION_LEVEL = PREFIX + "isolation_level";
public static final String TABLE_METADATA_CACHE_EXPIRATION_TIME_SECS =
PREFIX + "table_metadata.cache_expiration_time_secs";

public DatabaseConfig(File propertiesFile) throws IOException {
this(new FileInputStream(propertiesFile));
Expand Down Expand Up @@ -182,6 +186,11 @@ protected void load() {
if (!Strings.isNullOrEmpty(props.getProperty(ISOLATION_LEVEL))) {
isolation = Isolation.valueOf(props.getProperty(ISOLATION_LEVEL).toUpperCase());
}

if (!Strings.isNullOrEmpty(props.getProperty(TABLE_METADATA_CACHE_EXPIRATION_TIME_SECS))) {
tableMetadataCacheExpirationTimeSecs =
Long.parseLong(props.getProperty(TABLE_METADATA_CACHE_EXPIRATION_TIME_SECS));
}
}

public List<String> getContactPoints() {
Expand Down Expand Up @@ -224,4 +233,8 @@ public Class<? extends DistributedTransactionManager> getTransactionManagerClass
public Isolation getIsolation() {
return isolation;
}

public long getTableMetadataCacheExpirationTimeSecs() {
return tableMetadataCacheExpirationTimeSecs;
}
}
22 changes: 13 additions & 9 deletions core/src/main/java/com/scalar/db/storage/common/TableMetadataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import com.scalar.db.exception.storage.ExecutionException;
import java.util.Objects;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nonnull;

/** A class that manages and caches table metadata */
Expand All @@ -18,16 +19,19 @@ public class TableMetadataManager {
private final LoadingCache<TableKey, Optional<TableMetadata>> tableMetadataCache;

public TableMetadataManager(DistributedStorageAdmin admin, DatabaseConfig config) {
CacheBuilder<Object, Object> builder = CacheBuilder.newBuilder();
long tableMetadataCacheExpirationTimeSecs = config.getTableMetadataCacheExpirationTimeSecs();
if (tableMetadataCacheExpirationTimeSecs > 0) {
builder.expireAfterWrite(tableMetadataCacheExpirationTimeSecs, TimeUnit.SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sure about the use case of specifying the expiration time. Can you clarify?
From users' perspective, it seems like it should be cached forever or it should be replaced right after it needs to
be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The use case is when the table metadata change happens. As we don't have alter table feature for now, create table and delete table are only the use cases. Anyway, I think we will need to handle alter table cases in the future, so this cache expiration feature will become more important.

}
tableMetadataCache =
CacheBuilder.newBuilder()
.build(
new CacheLoader<TableKey, Optional<TableMetadata>>() {
@Override
public Optional<TableMetadata> load(@Nonnull TableKey key)
throws ExecutionException {
return Optional.ofNullable(admin.getTableMetadata(key.namespace, key.table));
}
});
builder.build(
new CacheLoader<TableKey, Optional<TableMetadata>>() {
@Override
public Optional<TableMetadata> load(@Nonnull TableKey key) throws ExecutionException {
return Optional.ofNullable(admin.getTableMetadata(key.namespace, key.table));
}
});
}

/**
Expand Down
26 changes: 26 additions & 0 deletions core/src/test/java/com/scalar/db/config/DatabaseConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public void constructor_PropertiesWithoutPortGiven_ShouldLoadProperly() {
assertThat(config.getNamespacePrefix()).isNotPresent();
assertThat(config.getTransactionManagerClass()).isEqualTo(ConsensusCommitManager.class);
assertThat(config.getIsolation()).isEqualTo(Isolation.SNAPSHOT);
assertThat(config.getTableMetadataCacheExpirationTimeSecs()).isEqualTo(-1);
}

@Test
Expand All @@ -76,6 +77,7 @@ public void constructor_PropertiesWithoutUsernameGiven_ShouldLoadProperly() {
assertThat(config.getNamespacePrefix()).isNotPresent();
assertThat(config.getTransactionManagerClass()).isEqualTo(ConsensusCommitManager.class);
assertThat(config.getIsolation()).isEqualTo(Isolation.SNAPSHOT);
assertThat(config.getTableMetadataCacheExpirationTimeSecs()).isEqualTo(-1);
}

@Test
Expand All @@ -100,6 +102,7 @@ public void constructor_PropertiesWithoutPasswordGiven_ShouldLoadProperly() {
assertThat(config.getNamespacePrefix()).isNotPresent();
assertThat(config.getTransactionManagerClass()).isEqualTo(ConsensusCommitManager.class);
assertThat(config.getIsolation()).isEqualTo(Isolation.SNAPSHOT);
assertThat(config.getTableMetadataCacheExpirationTimeSecs()).isEqualTo(-1);
}

@Test
Expand All @@ -126,6 +129,7 @@ public void constructor_PropertiesWithPortGiven_ShouldLoadProperly() {
assertThat(config.getNamespacePrefix()).isNotPresent();
assertThat(config.getTransactionManagerClass()).isEqualTo(ConsensusCommitManager.class);
assertThat(config.getIsolation()).isEqualTo(Isolation.SNAPSHOT);
assertThat(config.getTableMetadataCacheExpirationTimeSecs()).isEqualTo(-1);
}

@Test
Expand Down Expand Up @@ -442,4 +446,26 @@ public void constructor_UnsupportedIsolationGiven_ShouldThrowIllegalArgumentExce
assertThatThrownBy(() -> new DatabaseConfig(props))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
public void constructor_PropertiesWithTableMetadataExpirationTimeSecsGiven_ShouldLoadProperly() {
// Arrange
Properties props = new Properties();
props.setProperty(DatabaseConfig.CONTACT_POINTS, ANY_HOST);
props.setProperty(DatabaseConfig.USERNAME, ANY_USERNAME);
props.setProperty(DatabaseConfig.PASSWORD, ANY_PASSWORD);
props.setProperty(DatabaseConfig.TABLE_METADATA_CACHE_EXPIRATION_TIME_SECS, "3600");

// Act
DatabaseConfig config = new DatabaseConfig(props);

// Assert
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_HOST));
assertThat(config.getContactPort()).isEqualTo(0);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
assertThat(config.getTableMetadataCacheExpirationTimeSecs()).isEqualTo(3600);
}
}
37 changes: 37 additions & 0 deletions core/src/test/java/com/scalar/db/storage/common/TableMetadataManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.google.common.util.concurrent.Uninterruptibles;
import com.scalar.db.api.DistributedStorageAdmin;
import com.scalar.db.api.Get;
import com.scalar.db.api.TableMetadata;
Expand All @@ -13,6 +15,7 @@
import com.scalar.db.io.DataType;
import com.scalar.db.io.Key;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.util.concurrent.TimeUnit;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mock;
Expand All @@ -33,6 +36,7 @@ public void setUp() {
public void getTableMetadata_CalledOnce_ShouldCallDistributedStorageAdminOnce()
throws ExecutionException {
// Arrange
when(config.getTableMetadataCacheExpirationTimeSecs()).thenReturn(-1L);
TableMetadataManager tableMetadataManager = new TableMetadataManager(admin, config);

TableMetadata expectedTableMetadata =
Expand All @@ -50,6 +54,7 @@ public void getTableMetadata_CalledOnce_ShouldCallDistributedStorageAdminOnce()
new Get(new Key("c1", "aaa")).forNamespace("ns").forTable("tbl"));

// Assert
verify(config).getTableMetadataCacheExpirationTimeSecs();
verify(admin).getTableMetadata(anyString(), anyString());
assertThat(actualTableMetadata).isEqualTo(expectedTableMetadata);
}
Expand All @@ -58,6 +63,7 @@ public void getTableMetadata_CalledOnce_ShouldCallDistributedStorageAdminOnce()
public void getTableMetadata_CalledTwice_ShouldCallDistributedStorageAdminOnlyOnce()
throws ExecutionException {
// Arrange
when(config.getTableMetadataCacheExpirationTimeSecs()).thenReturn(-1L);
TableMetadataManager tableMetadataManager = new TableMetadataManager(admin, config);

TableMetadata expectedTableMetadata =
Expand All @@ -76,7 +82,38 @@ public void getTableMetadata_CalledTwice_ShouldCallDistributedStorageAdminOnlyOn
TableMetadata actualTableMetadata = tableMetadataManager.getTableMetadata(get);

// Assert
verify(config).getTableMetadataCacheExpirationTimeSecs();
verify(admin).getTableMetadata(anyString(), anyString());
assertThat(actualTableMetadata).isEqualTo(expectedTableMetadata);
}

@Test
public void getTableMetadata_CalledAfterCacheExpiration_ShouldCallDistributedStorageAdminAgain()
throws ExecutionException {
// Arrange
when(config.getTableMetadataCacheExpirationTimeSecs()).thenReturn(1L); // one second
TableMetadataManager tableMetadataManager = new TableMetadataManager(admin, config);

TableMetadata expectedTableMetadata =
TableMetadata.newBuilder()
.addColumn("c1", DataType.INT)
.addColumn("c2", DataType.INT)
.addPartitionKey("c1")
.build();

when(admin.getTableMetadata(anyString(), anyString())).thenReturn(expectedTableMetadata);

Get get = new Get(new Key("c1", "aaa")).forNamespace("ns").forTable("tbl");

// Act
tableMetadataManager.getTableMetadata(get);
// Wait for cache to be expired
Uninterruptibles.sleepUninterruptibly(1200, TimeUnit.MILLISECONDS);
TableMetadata actualTableMetadata = tableMetadataManager.getTableMetadata(get);

// Assert
verify(config).getTableMetadataCacheExpirationTimeSecs();
verify(admin, times(2)).getTableMetadata(anyString(), anyString());
assertThat(actualTableMetadata).isEqualTo(expectedTableMetadata);
}
}