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: commits-unsubscr...@cassandra.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org