frankgh commented on code in PR #200:
URL: https://github.com/apache/cassandra-sidecar/pull/200#discussion_r1966570408
##########
CONTRIBUTING.md:
##########
@@ -128,57 +159,94 @@ The `service` worker pool has the name
`sidecar-worker-pool`.
The `internal` worker pool has the name `sidecar-internal-worker-pool`.
-### <a href="timers"></a>One-shot Timers and Periodic Timers
+### <a name="timers"></a>One-shot Timers and Periodic Timers
Use vertx APIs to set one-shot timers and periodic timers. If you need to
execute a one-time operation in the future,
or if you need to run periodic operations within vertx, an API is available.
These timers utilize vertx provisioned
-threads that are managed internal by vertx. For example
+threads that are managed internal by vertx. For example, the below code
snippet runs `action()` after `delayMillis`.
```java
-logger.debug("Retrying streaming after {} millis", millis);
-executorPools.service().setTimer(millis, t -> acquireAndSend(context,
filename, fileLength, range, startTime));
+executorPools.service().setTimer(delayMillis, timerId -> action());
```
-### <a href="guice"></a>Dependency Injection
+Similarly, a simple periodic task can be schedule with the following code. The
`action()` is scheduled to run after
Review Comment:
```suggestion
Similarly, a simple periodic task can be scheduled with the following code.
The `action()` is scheduled to run after
```
##########
CONTRIBUTING.md:
##########
@@ -128,57 +159,94 @@ The `service` worker pool has the name
`sidecar-worker-pool`.
The `internal` worker pool has the name `sidecar-internal-worker-pool`.
-### <a href="timers"></a>One-shot Timers and Periodic Timers
+### <a name="timers"></a>One-shot Timers and Periodic Timers
Use vertx APIs to set one-shot timers and periodic timers. If you need to
execute a one-time operation in the future,
or if you need to run periodic operations within vertx, an API is available.
These timers utilize vertx provisioned
-threads that are managed internal by vertx. For example
+threads that are managed internal by vertx. For example, the below code
snippet runs `action()` after `delayMillis`.
```java
-logger.debug("Retrying streaming after {} millis", millis);
-executorPools.service().setTimer(millis, t -> acquireAndSend(context,
filename, fileLength, range, startTime));
+executorPools.service().setTimer(delayMillis, timerId -> action());
```
-### <a href="guice"></a>Dependency Injection
+Similarly, a simple periodic task can be schedule with the following code. The
`action()` is scheduled to run after
+the `initialDelayMillis` and repeat every `delayMillis`.
-The Apache Cassandra Sidecar project uses Guice for handling the object
interdependency. When a class encapsulates
-some functionality that is then used by a different class in the system, we
use Guice for dependency injection.
+```java
+executorPools.internal().setPeriodic(initialDelayMillis, delayMillis, timerId
-> action());
+```
+Note that such periodic task is triggered whenever the scheduled time has
arrived, consider the equivalent
+`ScheduledExecutorService#scheduleAtFixedRate`. It is possible to have
concurrent runs, depending on the delay parameters.
+It might not be the desired scheduling behavior for the use case. If serial
execution sequence is wanted, check out the
+scheduling mechanism described in [Advanced periodic task
scheduling](#advanced-periodic-scheduling)
-When a different implementation can be provided in the future, prefer using an
interface and a default implementation
-that can be later switched transparently without affecting the classes that
depend on it.
+#### <a name="advanced-periodic-scheduling">Advanced periodic task scheduling
-Prefer creating concrete classes over utility methods. The concrete classes
can be managed as a
-[Singleton](https://en.wikipedia.org/wiki/Singleton_pattern) if they do not
have external dependencies that might
-change their behavior based on the configuration.
+`PeriodicTaskExecutor` provides the scheduling behavior that is similar to the
hybrid of `ScheduledExecutorService#scheduleAtFixedRate`
+and `ScheduledExecutorService#scheduleAtFixedDelay`. The unit of execution is
`PeriodicTask`.
-Here's an example of a class being managed by Guice.
+A `PeriodicTask` is guaranteed to be executed in serial by
`PeriodicTaskExecutor`, as if such task is executed from
+a single thread. Memory consistency is ensured, i.e. the effect of write from
the last run is visible to the current run.
+The interval between the consecutive runs is adjusted based on the duration
taken by the last run. This way, its behavior
Review Comment:
I think we should not here that the duration is determined by the
PeriodicTask, if the future returns immediately, but there are background tasks
scheduled within the PeriodicTask, the duration of the actual task is not known
to the scheduler
##########
CHANGES.txt:
##########
@@ -1,5 +1,6 @@
0.1.0
-----
+ * Guice modularization (CASSSIDECAR-208)
Review Comment:
this feature should go under 0.2.0 instead
##########
server/src/main/java/org/apache/cassandra/sidecar/datahub/SchemaReportingTask.java:
##########
@@ -61,6 +63,13 @@ public SchemaReportingTask(@NotNull SidecarConfiguration
configuration,
this.reporter = reporter;
}
+ @Override
+ public void deploy(Vertx vertx, PeriodicTaskExecutor executor)
+ {
+ // TODO: react on ON_CASSANDRA_CQL_READY instead? When any CQL
connection is ready, cluster metadata should be available from session
Review Comment:
this actually sounds like a bug. If your Sidecar instance manages multiple
Cassandra instances, and your sidecar instance happens to be the cluster
leaseholder, this task will never schedule and you will never perform the
schema reporting unless another Sidecar instance takes over the cluster
leadership.
##########
server/src/main/java/org/apache/cassandra/sidecar/restore/RestoreJobManagerGroup.java:
##########
@@ -57,18 +56,12 @@ public class RestoreJobManagerGroup
public RestoreJobManagerGroup(SidecarConfiguration configuration,
InstancesMetadata instancesMetadata,
ExecutorPools executorPools,
- PeriodicTaskExecutor periodicTaskExecutor,
- RestoreProcessor restoreProcessor,
- RestoreJobDiscoverer jobDiscoverer,
- RingTopologyRefresher ringTopologyRefresher)
+ RestoreProcessor restoreProcessor)
{
this.restoreJobConfig = configuration.restoreJobConfiguration();
this.restoreProcessor = restoreProcessor;
this.executorPools = executorPools;
initializeManagers(instancesMetadata);
- periodicTaskExecutor.schedule(jobDiscoverer);
Review Comment:
nice 👍
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/authentication/ReloadingJwtAuthenticationHandler.java:
##########
@@ -174,6 +173,7 @@ public void execute(Promise<Void> promise)
if (!jwtParameters.enabled())
{
delegateHandler.set(null);
+ promise.complete();
Review Comment:
good catch
##########
server/src/main/java/org/apache/cassandra/sidecar/modules/guice-best-practice.md:
##########
@@ -0,0 +1,117 @@
+<!--
+#
+# 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.
+#
+-->
+
+# Guice Best Practices in Sidecar
+
+First of all, refer to [Guice
Wiki](https://github.com/google/guice/wiki/BestPractices) for the best
practices in general.
+
+There are certain project-specific best practices we advocate for Apache
Sidecar. This article capture those items in the below.
Review Comment:
```suggestion
There are certain project-specific best practices we advocate for Apache
Cassandra Sidecar. This article capture those items in the below.
```
##########
CONTRIBUTING.md:
##########
@@ -128,57 +159,94 @@ The `service` worker pool has the name
`sidecar-worker-pool`.
The `internal` worker pool has the name `sidecar-internal-worker-pool`.
-### <a href="timers"></a>One-shot Timers and Periodic Timers
+### <a name="timers"></a>One-shot Timers and Periodic Timers
Use vertx APIs to set one-shot timers and periodic timers. If you need to
execute a one-time operation in the future,
or if you need to run periodic operations within vertx, an API is available.
These timers utilize vertx provisioned
-threads that are managed internal by vertx. For example
+threads that are managed internal by vertx. For example, the below code
snippet runs `action()` after `delayMillis`.
```java
-logger.debug("Retrying streaming after {} millis", millis);
-executorPools.service().setTimer(millis, t -> acquireAndSend(context,
filename, fileLength, range, startTime));
+executorPools.service().setTimer(delayMillis, timerId -> action());
```
-### <a href="guice"></a>Dependency Injection
+Similarly, a simple periodic task can be schedule with the following code. The
`action()` is scheduled to run after
+the `initialDelayMillis` and repeat every `delayMillis`.
-The Apache Cassandra Sidecar project uses Guice for handling the object
interdependency. When a class encapsulates
-some functionality that is then used by a different class in the system, we
use Guice for dependency injection.
+```java
+executorPools.internal().setPeriodic(initialDelayMillis, delayMillis, timerId
-> action());
+```
+Note that such periodic task is triggered whenever the scheduled time has
arrived, consider the equivalent
+`ScheduledExecutorService#scheduleAtFixedRate`. It is possible to have
concurrent runs, depending on the delay parameters.
+It might not be the desired scheduling behavior for the use case. If serial
execution sequence is wanted, check out the
+scheduling mechanism described in [Advanced periodic task
scheduling](#advanced-periodic-scheduling)
-When a different implementation can be provided in the future, prefer using an
interface and a default implementation
-that can be later switched transparently without affecting the classes that
depend on it.
+#### <a name="advanced-periodic-scheduling">Advanced periodic task scheduling
Review Comment:
NIT: uppercase first letter for the title
```suggestion
#### <a name="advanced-periodic-scheduling">Advanced Periodic Task Scheduling
```
##########
server/src/main/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadataImpl.java:
##########
@@ -423,8 +423,8 @@ public InstanceMetadataImpl build()
if (storageDir == null)
{
- Objects.requireNonNull(dataDirs, "dataDirs are required when
storageDir is not configured");
- Preconditions.checkArgument(!dataDirs.isEmpty(), "dataDirs are
required when storageDir is not configured");
+ Preconditions.checkArgument(dataDirs != null &&
!dataDirs.isEmpty(),
Review Comment:
I think it's better to have an NPE for null input and
IllegalArgumentException for empty. Also this change is unrelated to this PR,
can you please revert?
##########
server/src/main/java/org/apache/cassandra/sidecar/modules/guice-best-practice.md:
##########
@@ -0,0 +1,117 @@
+<!--
+#
+# 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.
+#
+-->
+
+# Guice Best Practices in Sidecar
+
+First of all, refer to [Guice
Wiki](https://github.com/google/guice/wiki/BestPractices) for the best
practices in general.
+
+There are certain project-specific best practices we advocate for Apache
Sidecar. This article capture those items in the below.
+
+## Multi-bindings for Plugin Extension
+
+[Multi-bindings](https://github.com/google/guice/wiki/Multibindings) is a
Guice feature to support the plugin-type architecture,
+where multiple modules contribute components to plug into the system.
+
+The applications of multi-bindings in Sidecar are, for example,
+1. Routes definition
+2. Periodic tasks deployment
+3. Sidecar internal schemas creation
Review Comment:
I don't think this is scoped to sidecar internal schemas only, for example
system_auth is also managed by multi-bindings, as a read-only table
##########
server/src/test/java/org/apache/cassandra/sidecar/routes/sstableuploads/BaseUploadsHandlerTest.java:
##########
Review Comment:
all the tests under the `org.apache.cassandra.sidecar.routes` package should
move to the `org.apache.cassandra.sidecar.handlers` package
--
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-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
