You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "yifan-c (via GitHub)" <gi...@apache.org> on 2024/10/23 23:38:26 UTC
[PR] CASSANDRASC-152: Fix restore job of coordinated write not proceeding [cassandra-sidecar]
yifan-c opened a new pull request, #140:
URL: https://github.com/apache/cassandra-sidecar/pull/140
- Extends the restore_job table to persist the total slice_count in order to known the complete data to restore
- Allows external to trigger restore job discover to be more responsive
- Fix restore job & range status update
- Fix two phase import implementation in RestoreProcessor
- Expose overall restore progress status in API response
Patch by Yifan Cai; Reviewed by TBD for CASSANDRASC-152
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-152: Fix restore job of coordinated write not proceeding [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #140:
URL: https://github.com/apache/cassandra-sidecar/pull/140#discussion_r1815431534
##########
client-common/src/main/java/org/apache/cassandra/sidecar/common/data/ConsistencyVerificationResult.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.common.data;
+
+/**
+ * Verification result
+ */
+public enum ConsistencyVerificationResult
+{
+ SATISFIED, // the passed replicas have satisfied the consistency level
+ PENDING, // no conclusion can be made yet
+ FAILED, // the failed replicas have failed the consistency level
Review Comment:
these can be javadocs
##########
server/src/main/java/org/apache/cassandra/sidecar/restore/RestoreJobDiscoverer.java:
##########
@@ -154,11 +163,33 @@ public long delay()
@Override
public void execute(Promise<Void> promise)
{
- executeInternal();
+ tryExecute();
promise.tryComplete();
}
- public void executeInternal()
+ /**
+ * Try to execute the job discovery task in the blocking way. It returns immediately, if another thread is executing already.
+ * The method can be invoked by external call-sites.
+ */
+ public void tryExecute()
Review Comment:
NIT: maybe a more descriptive name is suitable here since it is in the public API
```suggestion
public void runDiscoverer()
```
##########
server/src/main/java/org/apache/cassandra/sidecar/restore/RestoreRangeTask.java:
##########
@@ -146,11 +146,11 @@ public void handle(Promise<RestoreRange> event)
RestoreJob job = range.job();
if (job.isManagedBySidecar())
{
- if (job.status == RestoreJobStatus.CREATED)
+ if (job.status == RestoreJobStatus.STAGE_READY)
{
if (Files.exists(range.stagedObjectPath()))
{
- LOGGER.debug("The slice has been staged already. sliceKey={} stagedFilePath={}",
+ LOGGER.info("The slice has been staged already. sliceKey={} stagedFilePath={}",
Review Comment:
info is desired here?
##########
server/src/main/java/org/apache/cassandra/sidecar/db/schema/AbstractSchema.java:
##########
@@ -44,7 +45,7 @@ public synchronized boolean initialize(@NotNull Session session)
protected PreparedStatement prepare(PreparedStatement cached, Session session, String cqlLiteral)
{
- return cached == null ? session.prepare(cqlLiteral) : cached;
+ return cached == null ? session.prepare(cqlLiteral).setConsistencyLevel(ConsistencyLevel.LOCAL_QUORUM) : cached;
Review Comment:
should CL be configurable?
##########
server/src/main/java/org/apache/cassandra/sidecar/db/RestoreRange.java:
##########
@@ -486,6 +487,30 @@ private <T> T readSliceProperty(Function<RestoreSlice, T> func)
return func.apply(source);
}
+ private void updateRangeStatusForInstance(InstanceMetadata instance, RestoreRangeStatus status)
+ {
+ // skip if the belong job is not managed by sidecar
+ if (!job().isManagedBySidecar())
+ {
+ return;
+ }
+
+ statusByReplica.put(storageAddressWithPort(instance), status);
+ }
+
+ private String storageAddressWithPort(InstanceMetadata instance)
+ {
+ try
+ {
+ InetSocketAddress storageAddress = instance.getFromDelegate(CassandraAdapterDelegate::localStorageBroadcastAddress);
+ return storageAddress.getAddress().getHostAddress() + ':' + storageAddress.getPort();
Review Comment:
will this work? I think internally it does something somewhat similar, with the added benefit that will check if addr != null
```suggestion
return return storageAddress.toString();
```
##########
server/src/main/java/org/apache/cassandra/sidecar/db/RestoreRange.java:
##########
@@ -147,7 +151,7 @@ private RestoreRange(Builder builder)
this.stagedObjectPath = builder.stagedObjectPath;
this.uploadId = builder.uploadId;
this.owner = builder.owner;
- this.statusByReplica = builder.statusByReplica;
+ this.statusByReplica = new HashMap<>(builder.statusByReplica);
Review Comment:
In the `org.apache.cassandra.sidecar.db.RestoreRange#statusByReplica` can we return `return Collections.unmodifiableMap(statusByReplica);` instead now. We don't want undesired modifications to an internal state
##########
server/src/main/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadata.java:
##########
@@ -58,10 +59,24 @@ public interface InstanceMetadata
/**
* @return a {@link CassandraAdapterDelegate} specific for the instance
*/
- @Nullable CassandraAdapterDelegate delegate();
+ @Nullable
+ CassandraAdapterDelegate delegate();
/**
* @return {@link InstanceMetrics} metrics specific for the Cassandra instance
*/
@NotNull InstanceMetrics metrics();
+
+ /**
+ * Get value from {@link CassandraAdapterDelegate}
+ * @param mapper the function is evaluated only when delegate is not null
+ * @return value retrieved from {@link CassandraAdapterDelegate} or null
+ * @param <T> value type
+ */
+ @Nullable
+ default <T> T getFromDelegate(Function<CassandraAdapterDelegate, T> mapper)
Review Comment:
nit:
```suggestion
default <T> T applyFromDelegate(Function<CassandraAdapterDelegate, T> mapper)
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-152: Fix restore job of coordinated write not proceeding [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #140:
URL: https://github.com/apache/cassandra-sidecar/pull/140#discussion_r1815790690
##########
server/src/main/java/org/apache/cassandra/sidecar/db/schema/AbstractSchema.java:
##########
@@ -44,7 +45,7 @@ public synchronized boolean initialize(@NotNull Session session)
protected PreparedStatement prepare(PreparedStatement cached, Session session, String cqlLiteral)
{
- return cached == null ? session.prepare(cqlLiteral) : cached;
+ return cached == null ? session.prepare(cqlLiteral).setConsistencyLevel(ConsistencyLevel.LOCAL_QUORUM) : cached;
Review Comment:
talked offline. not to be addressed in this PR.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-152: Fix restore job of coordinated write not proceeding [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c merged PR #140:
URL: https://github.com/apache/cassandra-sidecar/pull/140
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org