yifan-c commented on code in PR #31:
URL:
https://github.com/apache/cassandra-analytics/pull/31#discussion_r1456713939
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/data/CassandraDataLayer.java:
##########
@@ -341,24 +341,31 @@ private CompletionStage<Map<String, AvailabilityHint>>
createSnapshot(RingRespon
LOGGER.info("Creating snapshot on instance snapshotName={}
keyspace={} table={} datacenter={} fqdn={}",
snapshotName, maybeQuotedKeyspace,
maybeQuotedTable, datacenter, ringEntry.fqdn());
SidecarInstance sidecarInstance = new
SidecarInstanceImpl(ringEntry.fqdn(), sidecarClientConfig.effectivePort());
- createSnapshotFuture = sidecar
- .createSnapshot(sidecarInstance,
maybeQuotedKeyspace, maybeQuotedTable, snapshotName)
- .handle((resp, throwable) -> {
- if (throwable == null)
- {
- // Create snapshot succeeded
- return hint;
- }
-
- if (isExhausted(throwable))
- {
- LOGGER.warn("Failed to
create snapshot on instance", throwable);
- return
PartitionedDataLayer.AvailabilityHint.DOWN;
- }
-
- LOGGER.error("Unexpected error
creating snapshot on instance", throwable);
- return
PartitionedDataLayer.AvailabilityHint.UNKNOWN;
- });
+ String resolvedSnapshotTtl = options.clearSnapshot() ?
options.effectiveSnapshotTtl() : null;
Review Comment:
The condition is reversed. The resolved snapshot ttl is set, when
`!options.clearSnapshot()`.
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/data/ClientConfig.java:
##########
@@ -146,6 +158,16 @@ public boolean clearSnapshot()
return clearSnapshot;
}
+ public String userProvidedSnapshotTtl()
+ {
+ return userProvidedSnapshotTtl;
+ }
+
+ public String effectiveSnapshotTtl()
+ {
+ return effectiveSnapshotTtl;
+ }
Review Comment:
It looks like the `effectiveSnapshotTtl` is the resolved value on reading
from the map. It is confusing to expose both the raw and the resolved values.
Having `effectiveSnapshotTtl()` is suffice.
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/data/CassandraDataLayer.java:
##########
@@ -341,24 +341,31 @@ private CompletionStage<Map<String, AvailabilityHint>>
createSnapshot(RingRespon
LOGGER.info("Creating snapshot on instance snapshotName={}
keyspace={} table={} datacenter={} fqdn={}",
snapshotName, maybeQuotedKeyspace,
maybeQuotedTable, datacenter, ringEntry.fqdn());
SidecarInstance sidecarInstance = new
SidecarInstanceImpl(ringEntry.fqdn(), sidecarClientConfig.effectivePort());
- createSnapshotFuture = sidecar
- .createSnapshot(sidecarInstance,
maybeQuotedKeyspace, maybeQuotedTable, snapshotName)
- .handle((resp, throwable) -> {
- if (throwable == null)
- {
- // Create snapshot succeeded
- return hint;
- }
-
- if (isExhausted(throwable))
- {
- LOGGER.warn("Failed to
create snapshot on instance", throwable);
- return
PartitionedDataLayer.AvailabilityHint.DOWN;
- }
-
- LOGGER.error("Unexpected error
creating snapshot on instance", throwable);
- return
PartitionedDataLayer.AvailabilityHint.UNKNOWN;
- });
+ String resolvedSnapshotTtl = options.clearSnapshot() ?
options.effectiveSnapshotTtl() : null;
+ if (!options.clearSnapshot() &&
options.userProvidedSnapshotTtl() != null)
+ {
+ LOGGER.warn("Snapshot TTL option was provided along
with Clear Snapshot option, bulk reader" +
+ "can honor only one of the 2. Clear
Snapshot takes precedence over Snapshot TTL, " +
+ "hence snapshot {} will not be removed
after the specified snapshot TTL option", snapshotName);
+ }
Review Comment:
The condition is reversed too. Beside that, I think the warning is best
emitted from `ClientConfig` and use the resolved value at the call-sites only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
