You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "yifan-c (via GitHub)" <gi...@apache.org> on 2024/07/13 20:53:32 UTC
[PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
yifan-c opened a new pull request, #63:
URL: https://github.com/apache/cassandra-analytics/pull/63
This patch introduces a new option SIDECAR_CONTACT_POINTS for both bulk writer and reader. The option name better describes the purpose, which is to specify the initial contact points to discover the cluster topology. The existing option SIDECAR_INSTANCES are used for the same purpose and it is now deprecated. In addition, it allows including the port value in the addresses when defining SIDECAR_CONTACT_POINTS
Patch by Yifan Cai; Reviewed by Francisco Guerrero for CASSANDRA-19772
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63#discussion_r1680052228
##########
cassandra-analytics-core/src/test/spark3/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.spark.common;
+
+import org.junit.jupiter.api.Test;
+
+import org.apache.cassandra.sidecar.client.SidecarInstance;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+class SidecarInstanceFactoryTest
+{
+ @Test
+ void testCreateSidecarInstance()
+ {
+ assertThatThrownBy(() -> SidecarInstanceFactory.createFromString("", 9999))
+ .isExactlyInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Unable to create sidecar instance from empty input");
+
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost", 9999),
+ "localhost", 9999);
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("[2024:a::1]", 9999),
+ "[2024:a::1]", 9999);
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost:8888", 9999),
+ "localhost", 8888);
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("127.0.0.1:8888", 9999),
+ "127.0.0.1", 8888);
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("[2024:a::1]:8888", 9999),
Review Comment:
Sidecar client constructor `URI` for requests. It asks for bracketed IPv6 address.
> If a host is given then it is appended. If the host is a literal IPv6 address but is not enclosed in square brackets ('[' and ']') then the square brackets are added.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63#discussion_r1680053097
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/utils/MapUtils.java:
##########
@@ -239,4 +245,35 @@ public static boolean containsKey(Map<String, String> options, String key)
{
return options.containsKey(lowerCaseKey(key));
}
+
+ /**
+ * Resolve the value by consolidating the value associated with the current option and the deprecated option.
+ * The method assumes that both the new and the deprecated options expect the same value type.
+ * The value of the new option has the precedence over the deprecated one. Meaning when both options are present,
+ * the value associated with the new option is returned.
+ *
+ * @param options option keys and values map
+ * @param option new option
+ * @param deprecated deprecated option
+ * @param resolver function to resolve the value. If the input of the function is null, it asks for the default value
+ * @return resolved value
+ * @param <T> value type
+ */
+ public static <T> T resolveDeprecated(Map<String, String> options, String option, String deprecated, Function<String, T> resolver)
+ {
+ T deprecatedOptionValue = null;
+ if (options.containsKey(deprecated))
+ {
+ LOGGER.warn("The option: {} is deprecated. Please use {} instead. See option description for details.", deprecated, option);
+ deprecatedOptionValue = resolver.apply(deprecated);
+ }
+
+ if (options.containsKey(option))
+ {
+ LOGGER.info("The option: {} is defined. Favor the value over {}", option, deprecated);
Review Comment:
good idea
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63#discussion_r1680017755
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkSparkConf.java:
##########
@@ -146,17 +148,16 @@ public class BulkSparkConf implements Serializable
protected boolean useOpenSsl;
protected int ringRetryCount;
// create sidecarInstances from sidecarInstancesValue and effectiveSidecarPort
- private final String sidecarInstancesValue;
+ private final String sidecarContactPointsCsv;
Review Comment:
Not a huge fan of this variable name, can we use `sidecarContactPointsValue` instead?
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/utils/MapUtils.java:
##########
@@ -239,4 +245,35 @@ public static boolean containsKey(Map<String, String> options, String key)
{
return options.containsKey(lowerCaseKey(key));
}
+
+ /**
+ * Resolve the value by consolidating the value associated with the current option and the deprecated option.
+ * The method assumes that both the new and the deprecated options expect the same value type.
+ * The value of the new option has the precedence over the deprecated one. Meaning when both options are present,
+ * the value associated with the new option is returned.
+ *
+ * @param options option keys and values map
+ * @param option new option
+ * @param deprecated deprecated option
+ * @param resolver function to resolve the value. If the input of the function is null, it asks for the default value
+ * @return resolved value
+ * @param <T> value type
+ */
+ public static <T> T resolveDeprecated(Map<String, String> options, String option, String deprecated, Function<String, T> resolver)
+ {
+ T deprecatedOptionValue = null;
+ if (options.containsKey(deprecated))
+ {
+ LOGGER.warn("The option: {} is deprecated. Please use {} instead. See option description for details.", deprecated, option);
+ deprecatedOptionValue = resolver.apply(deprecated);
+ }
+
+ if (options.containsKey(option))
+ {
+ LOGGER.info("The option: {} is defined. Favor the value over {}", option, deprecated);
Review Comment:
info level here? maybe condition it to `deprecatedOptionValue` not `null`?
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/common/SidecarInstanceFactory.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.spark.common;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.sidecar.client.SidecarInstanceImpl;
+
+public class SidecarInstanceFactory
+{
+ private SidecarInstanceFactory()
+ {
+ throw new UnsupportedOperationException("Utility class");
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(SidecarInstanceFactory.class);
+
+ /**
+ * Create SidecarInstance object by parsing the input string, which is IP address or hostname and optionally includes port
+ * @param input hostname string that can optionally includes the port. If port is present, the defaultPort param is ignored.
+ * @param defaultPort port value used when the input string contains no port
+ * @return SidecarInstanceImpl
+ */
+ public static SidecarInstanceImpl createFromString(String input, int defaultPort)
+ {
+ Preconditions.checkArgument(StringUtils.isNotEmpty(input), "Unable to create sidecar instance from empty input");
Review Comment:
I think HostAndPort from guava is an alternative option to use. It is included as a transitive dependency from spark-core. I think we need to make sure the API is compatible across all spark version we support
##########
cassandra-analytics-core/src/test/spark3/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.spark.common;
+
+import org.junit.jupiter.api.Test;
+
+import org.apache.cassandra.sidecar.client.SidecarInstance;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+class SidecarInstanceFactoryTest
+{
+ @Test
+ void testCreateSidecarInstance()
+ {
+ assertThatThrownBy(() -> SidecarInstanceFactory.createFromString("", 9999))
+ .isExactlyInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Unable to create sidecar instance from empty input");
+
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost", 9999),
+ "localhost", 9999);
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("[2024:a::1]", 9999),
+ "[2024:a::1]", 9999);
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost:8888", 9999),
+ "localhost", 8888);
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("127.0.0.1:8888", 9999),
+ "127.0.0.1", 8888);
+ assertSidecarInstance(SidecarInstanceFactory.createFromString("[2024:a::1]:8888", 9999),
Review Comment:
I think we'll need to remove brackets from ipv6 addresses.
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkSparkConf.java:
##########
@@ -240,38 +241,47 @@ protected Set<String> buildBlockedInstances(Map<String, String> options)
.collect(Collectors.toSet());
}
- protected int resolveSSTableDataSizeInMiB(Map<String, String> options)
+ public static int resolveSSTableDataSizeInMiB(Map<String, String> options)
{
- int legacyOptionValue = -1;
- if (options.containsKey(WriterOptions.SSTABLE_DATA_SIZE_IN_MB.name()))
- {
- LOGGER.warn("The writer option: SSTABLE_DATA_SIZE_IN_MB is deprecated. " +
- "Please use SSTABLE_DATA_SIZE_IN_MIB instead. See option description for details.");
- legacyOptionValue = MapUtils.getInt(options, WriterOptions.SSTABLE_DATA_SIZE_IN_MB.name(),
- DEFAULT_SSTABLE_DATA_SIZE_IN_MIB, "sstable data size in mebibytes");
- }
+ return MapUtils.resolveDeprecated(options, WriterOptions.SSTABLE_DATA_SIZE_IN_MIB.name(), WriterOptions.SSTABLE_DATA_SIZE_IN_MB.name(), option -> {
+ if (option == null)
+ {
+ return DEFAULT_SSTABLE_DATA_SIZE_IN_MIB;
+ }
- if (options.containsKey(WriterOptions.SSTABLE_DATA_SIZE_IN_MIB.name()))
- {
- LOGGER.info("The writer option: SSTABLE_DATA_SIZE_IN_MIB is defined. " +
- "Favor the value over SSTABLE_DATA_SIZE_IN_MB");
- return MapUtils.getInt(options, WriterOptions.SSTABLE_DATA_SIZE_IN_MIB.name(),
- DEFAULT_SSTABLE_DATA_SIZE_IN_MIB, "sstable data size in mebibytes");
+ return MapUtils.getInt(options, option, DEFAULT_SSTABLE_DATA_SIZE_IN_MIB, "sstable data size in mebibytes");
+ });
+ }
- }
+ public static String resolveSidecarContactPoints(Map<String, String> options)
+ {
+ return MapUtils.resolveDeprecated(options, WriterOptions.SIDECAR_CONTACT_POINTS.name(), WriterOptions.SIDECAR_INSTANCES.name(), option -> {
+ if (option == null)
+ {
+ return null;
+ }
- return legacyOptionValue == -1 ? DEFAULT_SSTABLE_DATA_SIZE_IN_MIB : legacyOptionValue;
+ return MapUtils.getOrDefault(options, option, null);
+ });
}
protected Set<? extends SidecarInstance> buildSidecarInstances()
{
- String[] split = Objects.requireNonNull(sidecarInstancesValue, "Unable to build sidecar instances from null value")
+ String[] split = Objects.requireNonNull(sidecarContactPointsCsv, "Unable to build sidecar instances from null value")
.split(",");
return Arrays.stream(split)
- .map(hostname -> new SidecarInstanceImpl(hostname, effectiveSidecarPort))
+ .filter(StringUtils::isNotEmpty)
+ .map(hostname -> SidecarInstanceFactory.createFromString(hostname, effectiveSidecarPort))
.collect(Collectors.toSet());
}
+ // extract port from input if present and use the extract port to create SidecarInstanceImpl
+ // otherwise, the input does not contain port info, and it uses the default port
+ static SidecarInstanceImpl createSidecarInstance(String input, int defaultPort)
Review Comment:
unused method?
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63#discussion_r1680056218
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkSparkConf.java:
##########
@@ -240,38 +241,47 @@ protected Set<String> buildBlockedInstances(Map<String, String> options)
.collect(Collectors.toSet());
}
- protected int resolveSSTableDataSizeInMiB(Map<String, String> options)
+ public static int resolveSSTableDataSizeInMiB(Map<String, String> options)
{
- int legacyOptionValue = -1;
- if (options.containsKey(WriterOptions.SSTABLE_DATA_SIZE_IN_MB.name()))
- {
- LOGGER.warn("The writer option: SSTABLE_DATA_SIZE_IN_MB is deprecated. " +
- "Please use SSTABLE_DATA_SIZE_IN_MIB instead. See option description for details.");
- legacyOptionValue = MapUtils.getInt(options, WriterOptions.SSTABLE_DATA_SIZE_IN_MB.name(),
- DEFAULT_SSTABLE_DATA_SIZE_IN_MIB, "sstable data size in mebibytes");
- }
+ return MapUtils.resolveDeprecated(options, WriterOptions.SSTABLE_DATA_SIZE_IN_MIB.name(), WriterOptions.SSTABLE_DATA_SIZE_IN_MB.name(), option -> {
+ if (option == null)
+ {
+ return DEFAULT_SSTABLE_DATA_SIZE_IN_MIB;
+ }
- if (options.containsKey(WriterOptions.SSTABLE_DATA_SIZE_IN_MIB.name()))
- {
- LOGGER.info("The writer option: SSTABLE_DATA_SIZE_IN_MIB is defined. " +
- "Favor the value over SSTABLE_DATA_SIZE_IN_MB");
- return MapUtils.getInt(options, WriterOptions.SSTABLE_DATA_SIZE_IN_MIB.name(),
- DEFAULT_SSTABLE_DATA_SIZE_IN_MIB, "sstable data size in mebibytes");
+ return MapUtils.getInt(options, option, DEFAULT_SSTABLE_DATA_SIZE_IN_MIB, "sstable data size in mebibytes");
+ });
+ }
- }
+ public static String resolveSidecarContactPoints(Map<String, String> options)
+ {
+ return MapUtils.resolveDeprecated(options, WriterOptions.SIDECAR_CONTACT_POINTS.name(), WriterOptions.SIDECAR_INSTANCES.name(), option -> {
+ if (option == null)
+ {
+ return null;
+ }
- return legacyOptionValue == -1 ? DEFAULT_SSTABLE_DATA_SIZE_IN_MIB : legacyOptionValue;
+ return MapUtils.getOrDefault(options, option, null);
+ });
}
protected Set<? extends SidecarInstance> buildSidecarInstances()
{
- String[] split = Objects.requireNonNull(sidecarInstancesValue, "Unable to build sidecar instances from null value")
+ String[] split = Objects.requireNonNull(sidecarContactPointsCsv, "Unable to build sidecar instances from null value")
.split(",");
return Arrays.stream(split)
- .map(hostname -> new SidecarInstanceImpl(hostname, effectiveSidecarPort))
+ .filter(StringUtils::isNotEmpty)
+ .map(hostname -> SidecarInstanceFactory.createFromString(hostname, effectiveSidecarPort))
.collect(Collectors.toSet());
}
+ // extract port from input if present and use the extract port to create SidecarInstanceImpl
+ // otherwise, the input does not contain port info, and it uses the default port
+ static SidecarInstanceImpl createSidecarInstance(String input, int defaultPort)
Review Comment:
forgot to merge after switch to `SidecarInstanceFactory`
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c merged PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63#discussion_r1680058191
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkSparkConf.java:
##########
@@ -146,17 +148,16 @@ public class BulkSparkConf implements Serializable
protected boolean useOpenSsl;
protected int ringRetryCount;
// create sidecarInstances from sidecarInstancesValue and effectiveSidecarPort
- private final String sidecarInstancesValue;
+ private final String sidecarContactPointsCsv;
Review Comment:
ok.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63#discussion_r1680080380
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/common/SidecarInstanceFactory.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.spark.common;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.sidecar.client.SidecarInstanceImpl;
+
+public class SidecarInstanceFactory
+{
+ private SidecarInstanceFactory()
+ {
+ throw new UnsupportedOperationException("Utility class");
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(SidecarInstanceFactory.class);
+
+ /**
+ * Create SidecarInstance object by parsing the input string, which is IP address or hostname and optionally includes port
+ * @param input hostname string that can optionally includes the port. If port is present, the defaultPort param is ignored.
+ * @param defaultPort port value used when the input string contains no port
+ * @return SidecarInstanceImpl
+ */
+ public static SidecarInstanceImpl createFromString(String input, int defaultPort)
+ {
+ Preconditions.checkArgument(StringUtils.isNotEmpty(input), "Unable to create sidecar instance from empty input");
Review Comment:
Talked offline. We stick with the current implementation.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63#discussion_r1680072271
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/common/SidecarInstanceFactory.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.spark.common;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.StringUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.sidecar.client.SidecarInstanceImpl;
+
+public class SidecarInstanceFactory
+{
+ private SidecarInstanceFactory()
+ {
+ throw new UnsupportedOperationException("Utility class");
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(SidecarInstanceFactory.class);
+
+ /**
+ * Create SidecarInstance object by parsing the input string, which is IP address or hostname and optionally includes port
+ * @param input hostname string that can optionally includes the port. If port is present, the defaultPort param is ignored.
+ * @param defaultPort port value used when the input string contains no port
+ * @return SidecarInstanceImpl
+ */
+ public static SidecarInstanceImpl createFromString(String input, int defaultPort)
+ {
+ Preconditions.checkArgument(StringUtils.isNotEmpty(input), "Unable to create sidecar instance from empty input");
Review Comment:
Not sure if I follow. What is the benefit of creating a `HostAndPort` instance here? It cannot be used to construct `SidecarInstanceImpl`
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63#discussion_r1680162367
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/utils/MapUtils.java:
##########
@@ -239,4 +245,35 @@ public static boolean containsKey(Map<String, String> options, String key)
{
return options.containsKey(lowerCaseKey(key));
}
+
+ /**
+ * Resolve the value by consolidating the value associated with the current option and the deprecated option.
+ * The method assumes that both the new and the deprecated options expect the same value type.
+ * The value of the new option has the precedence over the deprecated one. Meaning when both options are present,
+ * the value associated with the new option is returned.
+ *
+ * @param options option keys and values map
+ * @param option new option
+ * @param deprecated deprecated option
+ * @param resolver function to resolve the value. If the input of the function is null, it asks for the default value
+ * @return resolved value
+ * @param <T> value type
+ */
+ public static <T> T resolveDeprecated(Map<String, String> options, String option, String deprecated, Function<String, T> resolver)
+ {
+ T deprecatedOptionValue = null;
+ if (options.containsKey(deprecated))
+ {
+ LOGGER.warn("The option: {} is deprecated. Please use {} instead. See option description for details.", deprecated, option);
+ deprecatedOptionValue = resolver.apply(deprecated);
+ }
+
+ if (options.containsKey(option) && deprecatedOptionValue != null)
+ {
+ LOGGER.info("The option: {} is defined. Favor the value over {}", option, deprecated);
Review Comment:
Wouldn't we be skipping this section altogether when the deprecated option is not provided?
```suggestion
if (options.containsKey(option))
{
if (deprecatedOptionValue != null)
{
LOGGER.info("The option: {} is defined. Favor the value over {}", option, deprecated);
}
```
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19772: Deprecate option SIDECAR_INSTANCES and replace with SIDECAR_CONTACT_POINTS [cassandra-analytics]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #63:
URL: https://github.com/apache/cassandra-analytics/pull/63#discussion_r1680162785
##########
scripts/build-sidecar.sh:
##########
@@ -24,7 +24,7 @@ else
SCRIPT_DIR=$( dirname -- "$( readlink -f -- "$0"; )"; )
SIDECAR_REPO="${SIDECAR_REPO:-https://github.com/apache/cassandra-sidecar.git}"
SIDECAR_BRANCH="${SIDECAR_BRANCH:-trunk}"
- SIDECAR_COMMIT="${SIDECAR_COMMIT:-e95786a077e1137dcaae206854986987edc6a71e}"
+ SIDECAR_COMMIT="${SIDECAR_COMMIT:-55a9efee30555d3645680c6524043a6c9bc1194b}"
Review Comment:
rebase maybe?
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org