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

Make Cosmos DTO objects immutable #1013

Merged
merged 3 commits into from
Aug 21, 2023
+255 −202
Merged

Make Cosmos DTO objects immutable #1013

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1bc6f6e
Remove setters of Cosmos marshalling objects
Torch3333 Aug 15, 2023
8b82fd7
Add @Immutable annotation
Torch3333 Aug 17, 2023
05a5c3c
Add comment about why the default constructor is needed
Torch3333 Aug 21, 2023
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
10 changes: 6 additions & 4 deletions core/src/integration-test/java/com/scalar/db/storage/cosmos/CosmosAdminTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@
import com.azure.cosmos.models.CosmosQueryRequestOptions;
import com.azure.cosmos.models.PartitionKey;
import com.azure.cosmos.util.CosmosPagedIterable;
import com.google.common.collect.ImmutableList;
import com.scalar.db.config.DatabaseConfig;
import com.scalar.db.util.AdminTestUtils;
import java.util.Properties;
import org.assertj.core.util.Sets;

public class CosmosAdminTestUtils extends AdminTestUtils {

Expand Down Expand Up @@ -65,9 +65,11 @@ record ->

@Override
public void corruptMetadata(String namespace, String table) {
CosmosTableMetadata corruptedMetadata = new CosmosTableMetadata();
corruptedMetadata.setId(getFullTableName(namespace, table));
corruptedMetadata.setPartitionKeyNames(ImmutableList.of("corrupted"));
CosmosTableMetadata corruptedMetadata =
CosmosTableMetadata.newBuilder()
.id(getFullTableName(namespace, table))
.partitionKeyNames(Sets.newLinkedHashSet("corrupted"))
.build();

CosmosContainer container =
client.getDatabase(metadataDatabase).getContainer(CosmosAdmin.METADATA_CONTAINER);
Expand Down
20 changes: 10 additions & 10 deletions core/src/main/java/com/scalar/db/storage/cosmos/CosmosAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,24 +243,24 @@ private CosmosContainer getMetadataContainer() {

private CosmosTableMetadata convertToCosmosTableMetadata(
String fullTableName, TableMetadata tableMetadata) {
CosmosTableMetadata cosmosTableMetadata = new CosmosTableMetadata();
cosmosTableMetadata.setId(fullTableName);
cosmosTableMetadata.setPartitionKeyNames(new ArrayList<>(tableMetadata.getPartitionKeyNames()));
cosmosTableMetadata.setClusteringKeyNames(
new ArrayList<>(tableMetadata.getClusteringKeyNames()));
cosmosTableMetadata.setClusteringOrders(
Map<String, String> clusteringOrders =
tableMetadata.getClusteringKeyNames().stream()
.collect(Collectors.toMap(c -> c, c -> tableMetadata.getClusteringOrder(c).name())));
cosmosTableMetadata.setSecondaryIndexNames(tableMetadata.getSecondaryIndexNames());
.collect(Collectors.toMap(c -> c, c -> tableMetadata.getClusteringOrder(c).name()));
Map<String, String> columnTypeByName = new HashMap<>();
tableMetadata
.getColumnNames()
.forEach(
columnName ->
columnTypeByName.put(
columnName, tableMetadata.getColumnDataType(columnName).name().toLowerCase()));
cosmosTableMetadata.setColumns(columnTypeByName);
return cosmosTableMetadata;
return CosmosTableMetadata.newBuilder()
.id(fullTableName)
.partitionKeyNames(tableMetadata.getPartitionKeyNames())
.clusteringKeyNames(tableMetadata.getClusteringKeyNames())
.clusteringOrders(clusteringOrders)
.secondaryIndexNames(tableMetadata.getSecondaryIndexNames())
.columns(columnTypeByName)
.build();
}

@Override
Expand Down
17 changes: 8 additions & 9 deletions core/src/main/java/com/scalar/db/storage/cosmos/CosmosMutation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import com.scalar.db.api.TableMetadata;
import com.scalar.db.io.Column;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
import javax.annotation.Nonnull;
import javax.annotation.concurrent.Immutable;
Expand Down Expand Up @@ -53,20 +54,18 @@ public MutationType getMutationType() {
@Nonnull
public Record makeRecord() {
Mutation mutation = (Mutation) getOperation();
Record record = new Record();

if (mutation instanceof Delete) {
return record;
return new Record();
}
Put put = (Put) mutation;

record.setId(getId());
record.setConcatenatedPartitionKey(getConcatenatedPartitionKey());
record.setPartitionKey(toMap(put.getPartitionKey().getColumns()));
put.getClusteringKey().ifPresent(k -> record.setClusteringKey(toMap(k.getColumns())));
record.setValues(toMapForPut(put));

return record;
return new Record(
getId(),
getConcatenatedPartitionKey(),
toMap(put.getPartitionKey().getColumns()),
put.getClusteringKey().map(k -> toMap(k.getColumns())).orElse(Collections.emptyMap()),
toMapForPut(put));
}

@Nonnull
Expand Down
116 changes: 86 additions & 30 deletions core/src/main/java/com/scalar/db/storage/cosmos/CosmosTableMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,52 +2,58 @@

import com.google.common.base.MoreObjects;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import javax.annotation.concurrent.NotThreadSafe;
import javax.annotation.Nullable;
import javax.annotation.concurrent.ThreadSafe;

/**
* A metadata class for a table of ScalarDB to know the type of each column
*
* @author Yuji Ito
*/
@SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
@NotThreadSafe
@ThreadSafe
Torch3333 marked this conversation as resolved.
Show resolved Hide resolved
public class CosmosTableMetadata {
Copy link
Contributor Author

@Torch3333 Torch3333 Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Remove setters and make attributes final here

private String id;
private LinkedHashSet<String> partitionKeyNames;
private LinkedHashSet<String> clusteringKeyNames;
private Map<String, String> clusteringOrders;
private Set<String> secondaryIndexNames;
private Map<String, String> columns;

public CosmosTableMetadata() {}

public void setId(String id) {
this.id = id;
}

public void setPartitionKeyNames(List<String> partitionKeyNames) {
this.partitionKeyNames = new LinkedHashSet<>(partitionKeyNames);
}

public void setClusteringKeyNames(List<String> clusteringKeyNames) {
this.clusteringKeyNames = new LinkedHashSet<>(clusteringKeyNames);
}

public void setClusteringOrders(Map<String, String> clusteringOrders) {
this.clusteringOrders = clusteringOrders;
private final String id;
private final LinkedHashSet<String> partitionKeyNames;
private final LinkedHashSet<String> clusteringKeyNames;
private final Map<String, String> clusteringOrders;
private final Set<String> secondaryIndexNames;
private final Map<String, String> columns;

public CosmosTableMetadata() {
this(null, null, null, null, null, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

This constructor is used only in unit tests (com.scalar.db.storage.cosmos.CosmosAdminTestBase#dropTable_WithMetadataLeft_ShouldDropContainerAndOnlyDeleteMetadata). How about making the test use the new constructor or adding @VisibleForTesting annotation here?

Copy link
Contributor Author

@Torch3333 Torch3333 Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Jackson requires a default constructor so we need it not only for testing purpose. I will add a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Where are we using Jackson databind for CosmosTableMetadata? I want to reproduce it and look into it as I'm a bit familiar with Jackson databind.

Copy link
Contributor

@komamitsu komamitsu Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ooops. Sorry, I missed the background.

This PR makes the CosmosMetadata and Record objects immutable. These DTO (Data Transfer Object) are used by the Cosmos SDK which uses Jackson.

Copy link
Contributor

Choose a reason for hiding this comment

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

JFYI: The following might be an option to work with Jackson databind internally used in Cosmos SDK, although it would make things a bit complicated.

  • build.gradle
    implementation 'com.fasterxml.jackson.core:jackson-databind:2.15.2'
  • CosmosTableMetadata.java
  public CosmosTableMetadata(
      @JsonProperty("id")
      @Nullable String id,
      @JsonProperty("partitionKeyNames")
      @Nullable LinkedHashSet<String> partitionKeyNames,
      @JsonProperty("clusteringKeyNames")
      @Nullable LinkedHashSet<String> clusteringKeyNames,
      @JsonProperty("clusteringOrders")
      @Nullable Map<String, String> clusteringOrders,
      @JsonProperty("secondaryIndexNames")
      @Nullable Set<String> secondaryIndexNames,
      @JsonProperty("columns")
      @Nullable Map<String, String> columns) {
    this.id = id != null ? id : "";
    this.partitionKeyNames = partitionKeyNames != null ? partitionKeyNames : new LinkedHashSet<>();
    ...
  }

I'm wondering why Cosmos SDK doesn't use https://github.com/FasterXML/jackson-modules-java8/tree/master/parameter-names or equivalent on their side...

Copy link
Contributor Author

@Torch3333 Torch3333 Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Thanks for sharing it.
Yes, it would be nice if the SDK were using that feature of Jackson.
Though, adding the jackson datababind dependency on our side to tinker with the Cosmos SDK default behavior seems a bit dangerous to me.

}

public void setSecondaryIndexNames(Set<String> secondaryIndexNames) {
this.secondaryIndexNames = secondaryIndexNames;
public CosmosTableMetadata(
@Nullable String id,
@Nullable LinkedHashSet<String> partitionKeyNames,
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be declared as an interface Set<String>.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah no, we can't do that because the insertion order is important for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Understood. (Another option might be simply using List with de-duplications)

@Nullable LinkedHashSet<String> clusteringKeyNames,
@Nullable Map<String, String> clusteringOrders,
@Nullable Set<String> secondaryIndexNames,
@Nullable Map<String, String> columns) {
this.id = id != null ? id : "";
this.partitionKeyNames = partitionKeyNames != null ? partitionKeyNames : new LinkedHashSet<>();
this.clusteringKeyNames =
clusteringKeyNames != null ? clusteringKeyNames : new LinkedHashSet<>();
this.clusteringOrders = clusteringOrders != null ? clusteringOrders : Collections.emptyMap();
this.secondaryIndexNames =
secondaryIndexNames != null ? secondaryIndexNames : Collections.emptySet();
this.columns = columns != null ? columns : Collections.emptyMap();
}

public void setColumns(Map<String, String> columns) {
this.columns = columns;
private CosmosTableMetadata(Builder builder) {
this(
builder.id,
builder.partitionKeyNames,
builder.clusteringKeyNames,
builder.clusteringOrders,
builder.secondaryIndexNames,
builder.columns);
}

public String getId() {
Expand Down Expand Up @@ -108,4 +114,54 @@ public String toString() {
.add("columns", columns)
.toString();
}

public static Builder newBuilder() {
return new Builder();
}

public static final class Builder {

private String id;
private LinkedHashSet<String> partitionKeyNames;
komamitsu marked this conversation as resolved.
Show resolved Hide resolved
private LinkedHashSet<String> clusteringKeyNames;
private Map<String, String> clusteringOrders;
private Set<String> secondaryIndexNames;
private Map<String, String> columns;

private Builder() {}

public Builder id(String val) {
id = val;
return this;
}

public Builder partitionKeyNames(LinkedHashSet<String> val) {
partitionKeyNames = val;
return this;
}

public Builder clusteringKeyNames(LinkedHashSet<String> val) {
clusteringKeyNames = val;
return this;
}

public Builder clusteringOrders(Map<String, String> val) {
clusteringOrders = val;
return this;
}

public Builder secondaryIndexNames(Set<String> val) {
secondaryIndexNames = val;
return this;
}

public Builder columns(Map<String, String> val) {
columns = val;
return this;
}

public CosmosTableMetadata build() {
return new CosmosTableMetadata(this);
}
}
}
49 changes: 23 additions & 26 deletions core/src/main/java/com/scalar/db/storage/cosmos/Record.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,42 +4,39 @@
import java.util.Collections;
import java.util.Map;
import java.util.Objects;
import javax.annotation.concurrent.NotThreadSafe;
import javax.annotation.Nullable;
import javax.annotation.concurrent.ThreadSafe;

/**
* A record class that Cosmos DB uses for storing a document based on ScalarDB data model.
*
* @author Yuji Ito
*/
@SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
@NotThreadSafe
@ThreadSafe
Torch3333 marked this conversation as resolved.
Show resolved Hide resolved
public class Record {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likewise

private String id = "";
private String concatenatedPartitionKey = "";
private Map<String, Object> partitionKey = Collections.emptyMap();
private Map<String, Object> clusteringKey = Collections.emptyMap();
private Map<String, Object> values = Collections.emptyMap();

public Record() {}

public void setId(String id) {
this.id = id;
}

public void setConcatenatedPartitionKey(String concatenatedPartitionKey) {
this.concatenatedPartitionKey = concatenatedPartitionKey;
}

public void setPartitionKey(Map<String, Object> partitionKey) {
this.partitionKey = partitionKey;
}

public void setClusteringKey(Map<String, Object> clusteringKey) {
this.clusteringKey = clusteringKey;
private final String id;
private final String concatenatedPartitionKey;
private final Map<String, Object> partitionKey;
private final Map<String, Object> clusteringKey;
private final Map<String, Object> values;

public Record() {
this(null, null, null, null, null);
}

public void setValues(Map<String, Object> values) {
this.values = values;
public Record(
@Nullable String id,
@Nullable String concatenatedPartitionKey,
@Nullable Map<String, Object> partitionKey,
@Nullable Map<String, Object> clusteringKey,
@Nullable Map<String, Object> values) {
this.id = id != null ? id : "";
this.concatenatedPartitionKey =
concatenatedPartitionKey != null ? concatenatedPartitionKey : "";
this.partitionKey = partitionKey != null ? partitionKey : Collections.emptyMap();
this.clusteringKey = clusteringKey != null ? clusteringKey : Collections.emptyMap();
this.values = values != null ? values : Collections.emptyMap();
}

public String getId() {
Expand Down
Loading