You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "jyothsnakonisa (via GitHub)" <gi...@apache.org> on 2025/04/09 17:57:12 UTC
[PR] CASSSIDECAR-236 Fixing incorrect creation of MinuteBoundConfiguration & MillisecondBoundConfiguration in CdcConfigImpl [cassandra-sidecar]
jyothsnakonisa opened a new pull request, #213:
URL: https://github.com/apache/cassandra-sidecar/pull/213
(no comment)
--
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] CASSSIDECAR-236 Fixing incorrect creation of MinuteBoundConfiguration & MillisecondBoundConfiguration in CdcConfigImpl [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #213:
URL: https://github.com/apache/cassandra-sidecar/pull/213#discussion_r2035948342
##########
server/src/test/java/org/apache/cassandra/sidecar/cdc/CdcConfigImplTest.java:
##########
@@ -137,6 +146,8 @@ void testConfigsWhenConfigsAreNotEmpty()
assertThat(cdcConfig.env()).isEqualTo("if");
assertThat(cdcConfig.kafkaTopic()).isEqualTo("topic1");
assertThat(cdcConfig.logOnly()).isFalse();
+ assertThat(cdcConfig.watermarkWindow()).isEqualTo(new MinuteBoundConfiguration(2, TimeUnit.MINUTES));
Review Comment:
and this. should it be `SecondBoundConfiguration`?
##########
server/src/test/java/org/apache/cassandra/sidecar/cdc/CdcConfigImplTest.java:
##########
@@ -118,6 +120,8 @@ void testReturnDefaultValuesWhenConfigsAreEmpty()
assertThat(cdcConfig.env()).isEqualTo("");
assertThat(cdcConfig.kafkaTopic()).isNull();
assertThat(cdcConfig.logOnly()).isFalse();
+ assertThat(cdcConfig.watermarkWindow()).isEqualTo(new MinuteBoundConfiguration(72, TimeUnit.HOURS));
Review Comment:
should this be `SecondBoundConfiguration`?
--
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] CASSSIDECAR-236 Fixing incorrect creation of MinuteBoundConfiguration & MillisecondBoundConfiguration in CdcConfigImpl [cassandra-sidecar]
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
jyothsnakonisa commented on code in PR #213:
URL: https://github.com/apache/cassandra-sidecar/pull/213#discussion_r2035931748
##########
server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcConfigImpl.java:
##########
@@ -179,7 +179,7 @@ public MinuteBoundConfiguration watermarkWindow()
{
// this prop sets the maximum duration age accepted by CDC, any mutations with write timestamps older than
// the watermark window will be dropped with log message "Exclude the update due to out of the allowed time window."
- return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW), TimeUnit.SECONDS);
+ return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW) / 60, TimeUnit.MINUTES);
Review Comment:
We can, but that might break any existing configs that are being used
--
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] CASSSIDECAR-236 Fixing incorrect creation of MinuteBoundConfiguration & MillisecondBoundConfiguration in CdcConfigImpl [cassandra-sidecar]
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
jyothsnakonisa commented on code in PR #213:
URL: https://github.com/apache/cassandra-sidecar/pull/213#discussion_r2035941112
##########
server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcConfigImpl.java:
##########
@@ -179,7 +179,7 @@ public MinuteBoundConfiguration watermarkWindow()
{
// this prop sets the maximum duration age accepted by CDC, any mutations with write timestamps older than
// the watermark window will be dropped with log message "Exclude the update due to out of the allowed time window."
- return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW), TimeUnit.SECONDS);
+ return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW) / 60, TimeUnit.MINUTES);
Review Comment:
I think changing the MinuteBound to SecondBound is better compared to changing configkey as it will break at compile time.
--
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] CASSSIDECAR-236 Fixing incorrect creation of MinuteBoundConfiguration & MillisecondBoundConfiguration in CdcConfigImpl [cassandra-sidecar]
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
jyothsnakonisa commented on code in PR #213:
URL: https://github.com/apache/cassandra-sidecar/pull/213#discussion_r2035894426
##########
server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcConfigImpl.java:
##########
@@ -179,7 +179,7 @@ public MinuteBoundConfiguration watermarkWindow()
{
// this prop sets the maximum duration age accepted by CDC, any mutations with write timestamps older than
// the watermark window will be dropped with log message "Exclude the update due to out of the allowed time window."
- return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW), TimeUnit.SECONDS);
+ return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW) / 60, TimeUnit.MINUTES);
Review Comment:
We can change that but should that be minute granularity or seconds granularity is the question. Also, note that it chages the API if we change it to SecondBound
--
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] CASSSIDECAR-236 Fixing incorrect creation of MinuteBoundConfiguration & MillisecondBoundConfiguration in CdcConfigImpl [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c merged PR #213:
URL: https://github.com/apache/cassandra-sidecar/pull/213
--
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] CASSSIDECAR-236 Fixing incorrect creation of MinuteBoundConfiguration & MillisecondBoundConfiguration in CdcConfigImpl [cassandra-sidecar]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on code in PR #213:
URL: https://github.com/apache/cassandra-sidecar/pull/213#discussion_r2035891235
##########
server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcConfigImpl.java:
##########
@@ -179,7 +179,7 @@ public MinuteBoundConfiguration watermarkWindow()
{
// this prop sets the maximum duration age accepted by CDC, any mutations with write timestamps older than
// the watermark window will be dropped with log message "Exclude the update due to out of the allowed time window."
- return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW), TimeUnit.SECONDS);
+ return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW) / 60, TimeUnit.MINUTES);
Review Comment:
Should we change the resolution to Seconds instead? What if someone configures 30 seconds for the watermark? That integer would be truncated to zero right?
--
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] CASSSIDECAR-236 Fixing incorrect creation of MinuteBoundConfiguration & MillisecondBoundConfiguration in CdcConfigImpl [cassandra-sidecar]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on code in PR #213:
URL: https://github.com/apache/cassandra-sidecar/pull/213#discussion_r2035905356
##########
server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcConfigImpl.java:
##########
@@ -179,7 +179,7 @@ public MinuteBoundConfiguration watermarkWindow()
{
// this prop sets the maximum duration age accepted by CDC, any mutations with write timestamps older than
// the watermark window will be dropped with log message "Exclude the update due to out of the allowed time window."
- return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW), TimeUnit.SECONDS);
+ return new MinuteBoundConfiguration(getInt(ConfigKeys.WATERMARK_SECONDS.lowcaseName, DEFAULT_WATERMARK_WINDOW) / 60, TimeUnit.MINUTES);
Review Comment:
Then we should change the ConfigKey to WATERMARK_MINUTES.
--
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