generate snapshot in background #364
Conversation
arcadiushmcts
force-pushed
the
dev/generate-snapshot-in-background
branch
8 times, most recently
from
1f65183 to
03bcb66
Compare
arcadiushmcts
requested review from
allanmckenzie,
santhosh-hmcts,
purple52 and
oktayekincioglu
| logger.debug("Stored successfully {}", storedSuccessfully); | ||
| return storedSuccessfully; | ||
| } catch (SerializationException e) { | ||
| logger.error("Error creating snapshot for %s".formatted(streamId), e); |
There was a problem hiding this comment.
Didn't know you could do this kind of formatting. Nice :-)
| @@ -26,7 +26,7 @@ public class AggregateSnapshotTest { | |||
| public void shouldCreateAnAggregateSnapshot() throws Exception { | |||
| final TestAggregate aggregate = new TestAggregate("STATE1"); | |||
|
|
|||
| final AggregateSnapshot<TestAggregate> snapshot = new AggregateSnapshot<>(STREAM_ID, VERSION_ID, aggregate); | |||
| final AggregateSnapshot<TestAggregate> snapshot = new AggregateSnapshot<>(STREAM_ID, VERSION_ID, aggregate, null); | |||
There was a problem hiding this comment.
I'm not sure I like passing in null like this. I've found nulls usually lead to tears and pain. Can this be an Optional instead?
| } | ||
|
|
||
| public AggregateSnapshot(final UUID streamId, final Long versionId, final String type, final byte[] aggregateByteRepresentation) { | ||
| public AggregateSnapshot(final UUID streamId, final Long versionId, final String type, final byte[] aggregateByteRepresentation, final ZonedDateTime createdAt) { |
There was a problem hiding this comment.
If the aggregateByteRepresentation byte array and createdAt date are linked, should they be but into a new single Object, AggregateByteArray or something. Also, if it's not always been created, should that Object be Optional?
arcadiushmcts
force-pushed
the
dev/generate-snapshot-in-background
branch
2 times, most recently
from
bf76db3 to
3c87402
Compare
| @@ -52,12 +59,18 @@ public class SnapshotJdbcRepository implements SnapshotRepository { | |||
| public boolean storeSnapshot(final AggregateSnapshot aggregateSnapshot) { | |||
|
|
|||
| try (final Connection connection = eventStoreDataSourceProvider.getDefaultDataSource().getConnection(); | |||
| final PreparedStatement ps = connection.prepareStatement(SQL_INSERT_EVENT_LOG)) { | |||
| final PreparedStatement ps = connection.prepareStatement(SQL_UPSERT_SNAPSHOT)) { | |||
| final Timestamp sqlTimestamp = toSqlTimestamp(clock.now()); | |||
There was a problem hiding this comment.
Better to rename this variable with what exactly it represents, may be something like nowTime?
|
|
||
| @Inject | ||
| @Value(key = "snapshot.background.saving.threshold", defaultValue = "" + SNAPSHOT_BACKGROUND_SAVING_THRESHOLD) | ||
| private long snapshotBackgroundSavingThreshold; |
There was a problem hiding this comment.
Depending on how this JNDI variable is going to be configured and if type is configured as java.lang.String auto conversion may not happen out of the box (I might be wrong with my assumption, worth checking it).
It's uncommon to define type as Long in standalone.xml at least by looking at existing practice. If this auto conversion won't happen and if JNDI variable type is configured as String, it may fail
Example of existing ocnfig practice:
There was a problem hiding this comment.
Thanks @santhosh-kumar3 for the comment.
On SIT:
name="java:/app/material-service/material.sasUrlExpiryInMinutes"
type="java.lang.String"
value="30"
/>
and the java code looks like this:
@Inject
@Value(key = "material.sasUrlExpiryInMinutes", defaultValue = "30")
private long expiryMinutes;
Also just for testing this, I have edited the docker project to add JNDI value
<simple name="java:/app/results-service/snapshot.background.saving.threshold" value="10000" type="java.lang.String"/>
and I have changed the code to log the value
logger.info("==== snapshotBackgroundSavingThreshold is {}", snapshotBackgroundSavingThreshold);
after redeploying results, I can see the output
==== snapshotBackgroundSavingThreshold is 10000
so, the automatic long/in conversion seems to be working
| return false; | ||
| } | ||
|
|
||
| final long threshold = snapshotBackgroundSavingThreshold <= 0 ? SNAPSHOT_BACKGROUND_SAVING_THRESHOLD : snapshotBackgroundSavingThreshold; |
There was a problem hiding this comment.
Is there any reason for checking for negative value? I suppose if variable is not configured it gets initialised to default value defined in class level static variable, so either it will have 0 or default value
There was a problem hiding this comment.
Correct, the threshold should never be zero (throws arithmetic-err : division-by-zero) and nothing is preventing a negative value
arcadiushmcts
force-pushed
the
dev/generate-snapshot-in-background
branch
2 times, most recently
from
62c11fc to
bfedc7e
Compare
arcadiushmcts
force-pushed
the
dev/generate-snapshot-in-background
branch
from
bfedc7e to
ceab499
Compare
|
No description provided.