Issue 1415 - DynamoDB store for table names & IDs #1418
Conversation
java/configuration/src/main/java/sleeper/configuration/properties/instance/CommonProperty.java
Outdated
Show resolved
Hide resolved
# Conflicts: # java/configuration/src/main/java/sleeper/configuration/properties/instance/CommonProperty.java # java/configuration/src/test/java/sleeper/configuration/properties/InstancePropertiesTestHelper.java
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class TableId { |
There was a problem hiding this comment.
Is this the right name for this class, given it contains a field called tableId? It means that TableIndex has the method Optional<TableId> getTableById(String tableId); which looks wrong. Could call this class TableIdAndName? Alternatively rename the field tableId to tableUniqueId and then the method on TableIndex would be Optional<TableId> getTableByUniqueId(String tableUniqueId);?
There was a problem hiding this comment.
I've renamed the field, thanks.
| @Test | ||
| void shouldGenerate32BitHexId() { | ||
| assertThat(TableIdGenerator.fromRandomSeed(0).generateString()) | ||
| .isEqualTo("60b420bb"); |
There was a problem hiding this comment.
Is it worth testing that a second call to the same TableIdGenerator gives a different string to the first one? There's nothing to stop someone reusing the same TableIdGenerator many times.
| .withExpressionAttributeNames(Map.of("#tablename", TABLE_NAME_FIELD))), | ||
| new TransactWriteItem().withPut(new Put() | ||
| .withTableName(idIndexDynamoTableName) | ||
| .withItem(DynamoDBTableIdFormat.getItem(id)) |
There was a problem hiding this comment.
As DynamoDBTableIdFormat.getItem(id) is used twice, I think it would be better to calculate it once and re-use it.
|
|
||
| @Override | ||
| public Stream<TableId> streamAllTables() { | ||
| return streamPagedItems(dynamoDB, new ScanRequest().withTableName(nameIndexDynamoTableName)) |
There was a problem hiding this comment.
We should probably use strongly consistent reads.
# Conflicts: # example/full/instance.properties # scripts/templates/instanceproperties.template
Make sure you have checked all steps below.
Issue
PR"
Tests
Documentation
separate issue for that below.
NOTICES file to reflect this.