You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "smiklosovic (via GitHub)" <gi...@apache.org> on 2023/06/29 15:30:32 UTC
[GitHub] [cassandra] smiklosovic opened a new pull request, #2458: CASSANDRA-18438
smiklosovic opened a new pull request, #2458:
URL: https://github.com/apache/cassandra/pull/2458
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257927646
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -61,6 +80,7 @@ public String apiCall(String url,
conn = (HttpURLConnection) new URL(url + query).openConnection();
extraHeaders.forEach(conn::setRequestProperty);
conn.setRequestMethod(method);
+ conn.setConnectTimeout(30000);
Review Comment:
well it is tricky to pass anything to these connectors from the properties as currently it is accepting metadata service url only. We might either refactor bunch of stuff for such a triviality or we can just have a system property in `CassandraRelevantProperties`? It is hard to believe this would be something which would be tweaked by users regularly, maybe for a power users as the ultimate way to weak it but otherwise ....
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259469586
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -61,6 +80,7 @@ public String apiCall(String url,
conn = (HttpURLConnection) new URL(url + query).openConnection();
extraHeaders.forEach(conn::setRequestProperty);
conn.setRequestMethod(method);
+ conn.setConnectTimeout(30000);
Review Comment:
since we have a generic metadata_url - why not just have a generic metadata_request_timeout in snitch properties? Though, I don't insist on this one 😃
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -61,6 +80,7 @@ public String apiCall(String url,
conn = (HttpURLConnection) new URL(url + query).openConnection();
extraHeaders.forEach(conn::setRequestProperty);
conn.setRequestMethod(method);
+ conn.setConnectTimeout(30000);
Review Comment:
since we have a generic metadata_url - why not just have a generic metadata_request_timeout in snitch properties? Though, I don't insist on this one 😃
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259494735
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -22,25 +22,55 @@
import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
+import java.net.MalformedURLException;
+import java.net.URISyntaxException;
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
+import org.apache.cassandra.exceptions.ConfigurationException;
+
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
Review Comment:
This property is owned by `GenericMDSSnitch` so maybe move it there?
--
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
[GitHub] [cassandra] smiklosovic closed pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #2458: CASSANDRA-18438
URL: https://github.com/apache/cassandra/pull/2458
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1247588494
##########
src/java/org/apache/cassandra/locator/AlibabaCloudSnitch.java:
##########
@@ -17,130 +17,47 @@
*/
package org.apache.cassandra.locator;
-import java.io.DataInputStream;
-import java.io.FilterInputStream;
import java.io.IOException;
-import java.net.HttpURLConnection;
-import java.net.MalformedURLException;
-import java.net.SocketTimeoutException;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.util.Map;
-import org.apache.cassandra.db.SystemKeyspace;
-import org.apache.cassandra.exceptions.ConfigurationException;
-import org.apache.cassandra.gms.ApplicationState;
-import org.apache.cassandra.gms.EndpointState;
-import org.apache.cassandra.gms.Gossiper;
-import org.apache.cassandra.io.util.FileUtils;
-import org.apache.cassandra.utils.FBUtilities;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
/**
- * A snitch that assumes an ECS region is a DC and an ECS availability_zone
- * is a rack. This information is available in the config for the node. the
- * format of the zone-id is like :cn-hangzhou-a where cn means china, hangzhou
- * means the hangzhou region, a means the az id. We use cn-hangzhou as the dc,
- * and f as the zone-id.
+ * A snitch that assumes an ECS region is a DC and an ECS availability_zone
+ * is a rack. This information is available in the config for the node. the
+ * format of the zone-id is like :cn-hangzhou-a where cn means china, hangzhou
+ * means the hangzhou region, a means the az id. We use cn-hangzhou as the dc,
+ * and f as the zone-id.
*/
-public class AlibabaCloudSnitch extends AbstractNetworkTopologySnitch
+public class AlibabaCloudSnitch extends AbstractCloudMetadataServiceSnitch
{
- protected static final Logger logger = LoggerFactory.getLogger(AlibabaCloudSnitch.class);
- protected static final String ZONE_NAME_QUERY_URL = "http://100.100.100.200/latest/meta-data/zone-id";
- private static final String DEFAULT_DC = "UNKNOWN-DC";
- private static final String DEFAULT_RACK = "UNKNOWN-RACK";
- private Map<InetAddressAndPort, Map<String, String>> savedEndpoints;
- protected String ecsZone;
- protected String ecsRegion;
-
- private static final int HTTP_CONNECT_TIMEOUT = 30000;
-
-
- public AlibabaCloudSnitch() throws MalformedURLException, IOException
- {
- String response = alibabaApiCall(ZONE_NAME_QUERY_URL);
- String[] splits = response.split("/");
- String az = splits[splits.length - 1];
-
- // Split "us-central1-a" or "asia-east1-a" into "us-central1"/"a" and "asia-east1"/"a".
- splits = az.split("-");
- ecsZone = splits[splits.length - 1];
-
- int lastRegionIndex = az.lastIndexOf("-");
- ecsRegion = az.substring(0, lastRegionIndex);
+ private static final String DEFAULT_METADATA_SERVICE_URL = "http://100.100.100.200";
+ private static final String ZONE_NAME_QUERY_URL = "/latest/meta-data/zone-id";
- String datacenterSuffix = (new SnitchProperties()).get("dc_suffix", "");
- ecsRegion = ecsRegion.concat(datacenterSuffix);
- logger.info("AlibabaSnitch using region: {}, zone: {}.", ecsRegion, ecsZone);
-
- }
-
- String alibabaApiCall(String url) throws ConfigurationException, IOException, SocketTimeoutException
+ public AlibabaCloudSnitch() throws IOException
{
- // Populate the region and zone by introspection, fail if 404 on metadata
- HttpURLConnection conn = (HttpURLConnection) new URL(url).openConnection();
- DataInputStream d = null;
- try
- {
- conn.setConnectTimeout(HTTP_CONNECT_TIMEOUT);
- conn.setRequestMethod("GET");
-
- int code = conn.getResponseCode();
- if (code != HttpURLConnection.HTTP_OK)
- throw new ConfigurationException("AlibabaSnitch was unable to execute the API call. Not an ecs node? and the returun code is " + code);
-
- // Read the information. I wish I could say (String) conn.getContent() here...
- int cl = conn.getContentLength();
- byte[] b = new byte[cl];
- d = new DataInputStream((FilterInputStream) conn.getContent());
- d.readFully(b);
- return new String(b, StandardCharsets.UTF_8);
- }
- catch (SocketTimeoutException e)
- {
- throw new SocketTimeoutException("Timeout occurred reading a response from the Alibaba ECS metadata");
- }
- finally
- {
- FileUtils.close(d);
- conn.disconnect();
- }
+ this(new SnitchProperties());
}
-
- @Override
- public String getRack(InetAddressAndPort endpoint)
+
+ public AlibabaCloudSnitch(SnitchProperties properties) throws IOException
{
- if (endpoint.equals(FBUtilities.getBroadcastAddressAndPort()))
- return ecsZone;
- EndpointState state = Gossiper.instance.getEndpointStateForEndpoint(endpoint);
- if (state == null || state.getApplicationState(ApplicationState.RACK) == null)
- {
- if (savedEndpoints == null)
- savedEndpoints = SystemKeyspace.loadDcRackInfo();
- if (savedEndpoints.containsKey(endpoint))
- return savedEndpoints.get(endpoint).get("rack");
- return DEFAULT_RACK;
- }
- return state.getApplicationState(ApplicationState.RACK).value;
-
+ this(properties, new DefaultCloudMetadataServiceConnector(properties.get(METADATA_URL_PROPERTY,
Review Comment:
See above comment about why it is split into two properties.
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259467444
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,41 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+
+ public static class DefaultCloudMetadataServiceConnector extends AbstractCloudMetadataServiceConnector
+ {
+ protected DefaultCloudMetadataServiceConnector(String metadataServiceUrl)
+ {
+ super(metadataServiceUrl);
+ }
+ }
+
protected final String metadataServiceUrl;
+ @VisibleForTesting // for mockito
+ protected String getMetadataServiceUrl()
+ {
+ return metadataServiceUrl;
+ }
+
protected AbstractCloudMetadataServiceConnector(String metadataServiceUrl)
{
+ Preconditions.checkNotNull(metadataServiceUrl, METADATA_URL_PROPERTY + " for a connector can not be null");
this.metadataServiceUrl = metadataServiceUrl;
}
public String apiCall(String query) throws IOException
{
- return apiCall(metadataServiceUrl, query, 200);
+ return apiCall(getMetadataServiceUrl(), query, 200);
Review Comment:
Nevertheless, I think this is not needed. I saw your mock and the reason why it fails with NPE is passing `anyString()` as url arg for `apiCall` - simply change it to `any()` and it will accept nulls.
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259776375
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -22,30 +22,77 @@
import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
+import java.net.MalformedURLException;
+import java.net.URISyntaxException;
import java.net.URL;
import java.util.Map;
import com.google.common.collect.ImmutableMap;
+import org.apache.cassandra.config.CassandraRelevantProperties;
+import org.apache.cassandra.exceptions.ConfigurationException;
+
+import static java.lang.String.format;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+ static final String METADATA_REQUEST_TIMEOUT_PROPRETY = "metadata_request_timeout";
Review Comment:
typo PROPRETY -> PROPERTY
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1258169132
##########
src/java/org/apache/cassandra/locator/Ec2MetadataServiceConnector.java:
##########
@@ -45,7 +45,7 @@ abstract class Ec2MetadataServiceConnector extends AbstractCloudMetadataServiceC
Ec2MetadataServiceConnector(SnitchProperties properties)
{
- super(properties.get(EC2_METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL));
+ super(properties.get(EC2_METADATA_URL_PROPERTY, properties.get(METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL)));
Review Comment:
perhaps we should not bother that much - who would set it to something else than a default value?
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257927646
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -61,6 +80,7 @@ public String apiCall(String url,
conn = (HttpURLConnection) new URL(url + query).openConnection();
extraHeaders.forEach(conn::setRequestProperty);
conn.setRequestMethod(method);
+ conn.setConnectTimeout(30000);
Review Comment:
well it is tricky to pass anything to these connectors from the properties as currently it is accepting metadata service url only. We might either refactor bunch of stuff for such a triviality or we can just have a system property in `CassandraRelevantProperties`? It is hard to believe this would be something which would be tweaked by users regularly, maybe for power users as the ultimate way to weak it but otherwise ....
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1258186447
##########
src/java/org/apache/cassandra/locator/Ec2MetadataServiceConnector.java:
##########
@@ -45,7 +45,7 @@ abstract class Ec2MetadataServiceConnector extends AbstractCloudMetadataServiceC
Ec2MetadataServiceConnector(SnitchProperties properties)
{
- super(properties.get(EC2_METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL));
+ super(properties.get(EC2_METADATA_URL_PROPERTY, properties.get(METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL)));
Review Comment:
anyway it does not hurt if it is here.
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1254197140
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.locator;
+
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.db.SystemKeyspace;
+import org.apache.cassandra.gms.ApplicationState;
+import org.apache.cassandra.gms.EndpointState;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.utils.FBUtilities;
+
+abstract class AbstractCloudMetadataServiceSnitch extends AbstractNetworkTopologySnitch
+{
+ static final Logger logger = LoggerFactory.getLogger(AbstractCloudMetadataServiceSnitch.class);
+
+ static final String DEFAULT_DC = "UNKNOWN-DC";
+ static final String DEFAULT_RACK = "UNKNOWN-RACK";
+
+ protected final AbstractCloudMetadataServiceConnector connector;
+ protected final SnitchProperties snitchProperties;
+
+ protected String localRack;
Review Comment:
So my preference is that those two fields are private final in the subclasses and in this class we only have abstract methods like `getLocalRack()` and `getLocalDC()` which are called from `getRack(endpoint)` and `getDatacenter(endpoint)`
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.locator;
+
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.db.SystemKeyspace;
+import org.apache.cassandra.gms.ApplicationState;
+import org.apache.cassandra.gms.EndpointState;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.utils.FBUtilities;
+
+abstract class AbstractCloudMetadataServiceSnitch extends AbstractNetworkTopologySnitch
+{
+ static final Logger logger = LoggerFactory.getLogger(AbstractCloudMetadataServiceSnitch.class);
+
+ static final String DEFAULT_DC = "UNKNOWN-DC";
+ static final String DEFAULT_RACK = "UNKNOWN-RACK";
+
+ protected final AbstractCloudMetadataServiceConnector connector;
+ protected final SnitchProperties snitchProperties;
+
+ protected String localRack;
+ protected String localDc;
+
+ private Map<InetAddressAndPort, Map<String, String>> savedEndpoints;
+
+ public AbstractCloudMetadataServiceSnitch(AbstractCloudMetadataServiceConnector connector, SnitchProperties snitchProperties)
+ {
+ this.connector = connector;
+ this.snitchProperties = snitchProperties;
+ }
+
+ @Override
+ public String getRack(InetAddressAndPort endpoint)
+ {
+ if (endpoint.equals(FBUtilities.getBroadcastAddressAndPort()))
+ return localRack;
+ EndpointState state = Gossiper.instance.getEndpointStateForEndpoint(endpoint);
+ if (state == null || state.getApplicationState(ApplicationState.RACK) == null)
+ {
+ if (savedEndpoints == null)
+ savedEndpoints = SystemKeyspace.loadDcRackInfo();
+ if (savedEndpoints.containsKey(endpoint))
+ return savedEndpoints.get(endpoint).get("rack");
+ return DEFAULT_RACK;
+ }
+ return state.getApplicationState(ApplicationState.RACK).value;
+
+ }
+
+ @Override
+ public String getDatacenter(InetAddressAndPort endpoint)
+ {
+ if (endpoint.equals(FBUtilities.getBroadcastAddressAndPort()))
+ return localDc;
+ EndpointState state = Gossiper.instance.getEndpointStateForEndpoint(endpoint);
+ if (state == null || state.getApplicationState(ApplicationState.DC) == null)
+ {
+ if (savedEndpoints == null)
+ savedEndpoints = SystemKeyspace.loadDcRackInfo();
+ if (savedEndpoints.containsKey(endpoint))
+ return savedEndpoints.get(endpoint).get("data_center");
+ return DEFAULT_DC;
+ }
+ return state.getApplicationState(ApplicationState.DC).value;
+ }
+
+ protected void parseResponse(String response)
Review Comment:
I think that this method does not belong to this abstract implementation. There should be no details about the response interpretation. If this method is used in many places, perhaps it could go as static to something like `SnitchUtils`.
##########
src/java/org/apache/cassandra/locator/DefaultCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.locator;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
+
+/**
+ * Default cloud-based metadata service snitch which parses datacenter and rack of a node by
+ * executing GET query for URL configured in cassandra-rackdc.properties under
+ * key {@link AbstractCloudMetadataServiceConnector#METADATA_URL_PROPERTY}. It expects HTTP response code 200.
+ * It is not passing any headers to GET request.
+ * <p>
+ * The response will be parsed like following - if the response body is, for example "us-central1-a", then
+ * the datacenter will be "us-central1" and rack will be "a". There is value of "dc_suffix" key in
+ * cassandra-rackdc.properties appended to datacenter.
+ */
+public class DefaultCloudMetadataServiceSnitch extends AbstractCloudMetadataServiceSnitch
+{
+ public DefaultCloudMetadataServiceSnitch() throws IOException
+ {
+ this(new SnitchProperties());
+ }
+
+ public DefaultCloudMetadataServiceSnitch(SnitchProperties properties) throws IOException
Review Comment:
maybe let it accept the default url as argument so that the inheritors can set it directly?
##########
src/java/org/apache/cassandra/locator/DefaultCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.locator;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
+
+/**
+ * Default cloud-based metadata service snitch which parses datacenter and rack of a node by
+ * executing GET query for URL configured in cassandra-rackdc.properties under
+ * key {@link AbstractCloudMetadataServiceConnector#METADATA_URL_PROPERTY}. It expects HTTP response code 200.
+ * It is not passing any headers to GET request.
+ * <p>
+ * The response will be parsed like following - if the response body is, for example "us-central1-a", then
+ * the datacenter will be "us-central1" and rack will be "a". There is value of "dc_suffix" key in
+ * cassandra-rackdc.properties appended to datacenter.
+ */
+public class DefaultCloudMetadataServiceSnitch extends AbstractCloudMetadataServiceSnitch
+{
Review Comment:
would you rename it into something like `GenericCloudMetadataServiceSnitch`?
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,41 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+
+ public static class DefaultCloudMetadataServiceConnector extends AbstractCloudMetadataServiceConnector
+ {
+ protected DefaultCloudMetadataServiceConnector(String metadataServiceUrl)
+ {
+ super(metadataServiceUrl);
+ }
+ }
+
protected final String metadataServiceUrl;
+ @VisibleForTesting // for mockito
+ protected String getMetadataServiceUrl()
+ {
+ return metadataServiceUrl;
+ }
+
protected AbstractCloudMetadataServiceConnector(String metadataServiceUrl)
{
+ Preconditions.checkNotNull(metadataServiceUrl, METADATA_URL_PROPERTY + " for a connector can not be null");
this.metadataServiceUrl = metadataServiceUrl;
}
public String apiCall(String query) throws IOException
{
- return apiCall(metadataServiceUrl, query, 200);
+ return apiCall(getMetadataServiceUrl(), query, 200);
Review Comment:
why?
##########
src/java/org/apache/cassandra/locator/DefaultCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.locator;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
+
+/**
+ * Default cloud-based metadata service snitch which parses datacenter and rack of a node by
+ * executing GET query for URL configured in cassandra-rackdc.properties under
+ * key {@link AbstractCloudMetadataServiceConnector#METADATA_URL_PROPERTY}. It expects HTTP response code 200.
+ * It is not passing any headers to GET request.
+ * <p>
+ * The response will be parsed like following - if the response body is, for example "us-central1-a", then
+ * the datacenter will be "us-central1" and rack will be "a". There is value of "dc_suffix" key in
+ * cassandra-rackdc.properties appended to datacenter.
+ */
+public class DefaultCloudMetadataServiceSnitch extends AbstractCloudMetadataServiceSnitch
Review Comment:
does this class need to be public?
##########
src/java/org/apache/cassandra/locator/Ec2MetadataServiceConnector.java:
##########
@@ -45,7 +45,7 @@ abstract class Ec2MetadataServiceConnector extends AbstractCloudMetadataServiceC
Ec2MetadataServiceConnector(SnitchProperties properties)
{
- super(properties.get(EC2_METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL));
+ super(properties.get(EC2_METADATA_URL_PROPERTY, properties.get(METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL)));
Review Comment:
why do we need this?
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,41 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
Review Comment:
CassandraRelevantProperties?
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,41 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+
+ public static class DefaultCloudMetadataServiceConnector extends AbstractCloudMetadataServiceConnector
+ {
+ protected DefaultCloudMetadataServiceConnector(String metadataServiceUrl)
+ {
+ super(metadataServiceUrl);
+ }
+ }
+
protected final String metadataServiceUrl;
+ @VisibleForTesting // for mockito
+ protected String getMetadataServiceUrl()
+ {
+ return metadataServiceUrl;
+ }
+
protected AbstractCloudMetadataServiceConnector(String metadataServiceUrl)
{
+ Preconditions.checkNotNull(metadataServiceUrl, METADATA_URL_PROPERTY + " for a connector can not be null");
Review Comment:
Since you do validation, maybe also validate if it is a valid URL?
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -61,6 +80,7 @@ public String apiCall(String url,
conn = (HttpURLConnection) new URL(url + query).openConnection();
extraHeaders.forEach(conn::setRequestProperty);
conn.setRequestMethod(method);
+ conn.setConnectTimeout(30000);
Review Comment:
Should we take the timeout from the properties with 30s as default?
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259485735
##########
src/java/org/apache/cassandra/locator/DefaultCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.locator;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
+
+/**
+ * Default cloud-based metadata service snitch which parses datacenter and rack of a node by
+ * executing GET query for URL configured in cassandra-rackdc.properties under
+ * key {@link AbstractCloudMetadataServiceConnector#METADATA_URL_PROPERTY}. It expects HTTP response code 200.
+ * It is not passing any headers to GET request.
+ * <p>
+ * The response will be parsed like following - if the response body is, for example "us-central1-a", then
+ * the datacenter will be "us-central1" and rack will be "a". There is value of "dc_suffix" key in
+ * cassandra-rackdc.properties appended to datacenter.
+ */
+public class DefaultCloudMetadataServiceSnitch extends AbstractCloudMetadataServiceSnitch
+{
+ public DefaultCloudMetadataServiceSnitch() throws IOException
+ {
+ this(new SnitchProperties());
+ }
+
+ public DefaultCloudMetadataServiceSnitch(SnitchProperties properties) throws IOException
Review Comment:
ok, let's leave it as it is
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259492824
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -22,25 +22,55 @@
import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
+import java.net.MalformedURLException;
+import java.net.URISyntaxException;
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
+import org.apache.cassandra.exceptions.ConfigurationException;
+
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+
+ public static class DefaultCloudMetadataServiceConnector extends AbstractCloudMetadataServiceConnector
+ {
+ protected DefaultCloudMetadataServiceConnector(String metadataServiceUrl)
+ {
+ super(metadataServiceUrl);
+ }
+ }
+
protected final String metadataServiceUrl;
+ @VisibleForTesting // for mockito
+ protected String getMetadataServiceUrl()
+ {
+ return metadataServiceUrl;
+ }
+
protected AbstractCloudMetadataServiceConnector(String metadataServiceUrl)
{
+ try
+ {
+ URL url = new URL(metadataServiceUrl);
+ url.toURI();
+ }
+ catch (MalformedURLException | IllegalArgumentException | URISyntaxException ex)
+ {
+ throw new ConfigurationException(String.format("URL you provided (%s) is not valid!", metadataServiceUrl), ex);
+ }
this.metadataServiceUrl = metadataServiceUrl;
}
public String apiCall(String query) throws IOException
{
- return apiCall(metadataServiceUrl, query, 200);
+ return apiCall(getMetadataServiceUrl(), query, 200);
}
public String apiCall(String url, String query, int expectedResponseCode) throws IOException
Review Comment:
I think this method should be converted into:
```
apiCall(String query, Map<String, String> extraHeaders)
```
and then used, for example in Google snitch.
In general, we should avoid using `apiCall` which accepts url - it should be reserved only for extensions. That is, the other `apiCall` overloads should be made final.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259557672
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -22,25 +22,55 @@
import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
+import java.net.MalformedURLException;
+import java.net.URISyntaxException;
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
+import org.apache.cassandra.exceptions.ConfigurationException;
+
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
Review Comment:
I dont think it is owned by it. It is good where it is imho.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259542990
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -61,6 +80,7 @@ public String apiCall(String url,
conn = (HttpURLConnection) new URL(url + query).openConnection();
extraHeaders.forEach(conn::setRequestProperty);
conn.setRequestMethod(method);
+ conn.setConnectTimeout(30000);
Review Comment:
Anyway, I think I will do it in such a way that we will pass SnitchProperties to connector. Because my gut feeling tells me that there might be connectors which might be somehow further configured. Now it is pretty simple, just metadata url and possibly timeout, but what if connectors need something fancy? I dont know ... certificates? You feel me. So there needs to be a way how to get the access to all snitch properties.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259542990
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -61,6 +80,7 @@ public String apiCall(String url,
conn = (HttpURLConnection) new URL(url + query).openConnection();
extraHeaders.forEach(conn::setRequestProperty);
conn.setRequestMethod(method);
+ conn.setConnectTimeout(30000);
Review Comment:
Anyway, I think I will do it in such a way that we will pass SnitchProperties to connector. Because my gut feeling tells me that there might be connectors which might be somehow further configured. Now it is pretty simple, just metadata url and possibly timeout, but what if connectors need something fancy? I dont know ... certificates? You feel me. So the needs to be a way how to get the access to all snitch properties.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257922508
##########
src/java/org/apache/cassandra/locator/DefaultCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.locator;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
+
+/**
+ * Default cloud-based metadata service snitch which parses datacenter and rack of a node by
+ * executing GET query for URL configured in cassandra-rackdc.properties under
+ * key {@link AbstractCloudMetadataServiceConnector#METADATA_URL_PROPERTY}. It expects HTTP response code 200.
+ * It is not passing any headers to GET request.
+ * <p>
+ * The response will be parsed like following - if the response body is, for example "us-central1-a", then
+ * the datacenter will be "us-central1" and rack will be "a". There is value of "dc_suffix" key in
+ * cassandra-rackdc.properties appended to datacenter.
+ */
+public class DefaultCloudMetadataServiceSnitch extends AbstractCloudMetadataServiceSnitch
+{
+ public DefaultCloudMetadataServiceSnitch() throws IOException
+ {
+ this(new SnitchProperties());
+ }
+
+ public DefaultCloudMetadataServiceSnitch(SnitchProperties properties) throws IOException
Review Comment:
I don't think that is really necessary.
I can do this though.
public DefaultCloudMetadataServiceSnitch(String url) throws IOException
{
this(new SnitchProperties(Pair.create(METADATA_URL_PROPERTY, url)));
}
If you look how the response is parsed etc, we also need dc suffix propagated and in case like I did it it would not be parsed from property file.
I can also do this
public DefaultCloudMetadataServiceSnitch(SnitchProperties properties, String url) throws IOException
{
this(properties, new DefaultCloudMetadataServiceConnector(url));
}
But why would you do that if it is going to be taken from `properties` anyway as we need them?
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1247588494
##########
src/java/org/apache/cassandra/locator/AlibabaCloudSnitch.java:
##########
@@ -17,130 +17,47 @@
*/
package org.apache.cassandra.locator;
-import java.io.DataInputStream;
-import java.io.FilterInputStream;
import java.io.IOException;
-import java.net.HttpURLConnection;
-import java.net.MalformedURLException;
-import java.net.SocketTimeoutException;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.util.Map;
-import org.apache.cassandra.db.SystemKeyspace;
-import org.apache.cassandra.exceptions.ConfigurationException;
-import org.apache.cassandra.gms.ApplicationState;
-import org.apache.cassandra.gms.EndpointState;
-import org.apache.cassandra.gms.Gossiper;
-import org.apache.cassandra.io.util.FileUtils;
-import org.apache.cassandra.utils.FBUtilities;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
/**
- * A snitch that assumes an ECS region is a DC and an ECS availability_zone
- * is a rack. This information is available in the config for the node. the
- * format of the zone-id is like :cn-hangzhou-a where cn means china, hangzhou
- * means the hangzhou region, a means the az id. We use cn-hangzhou as the dc,
- * and f as the zone-id.
+ * A snitch that assumes an ECS region is a DC and an ECS availability_zone
+ * is a rack. This information is available in the config for the node. the
+ * format of the zone-id is like :cn-hangzhou-a where cn means china, hangzhou
+ * means the hangzhou region, a means the az id. We use cn-hangzhou as the dc,
+ * and f as the zone-id.
*/
-public class AlibabaCloudSnitch extends AbstractNetworkTopologySnitch
+public class AlibabaCloudSnitch extends AbstractCloudMetadataServiceSnitch
{
- protected static final Logger logger = LoggerFactory.getLogger(AlibabaCloudSnitch.class);
- protected static final String ZONE_NAME_QUERY_URL = "http://100.100.100.200/latest/meta-data/zone-id";
- private static final String DEFAULT_DC = "UNKNOWN-DC";
- private static final String DEFAULT_RACK = "UNKNOWN-RACK";
- private Map<InetAddressAndPort, Map<String, String>> savedEndpoints;
- protected String ecsZone;
- protected String ecsRegion;
-
- private static final int HTTP_CONNECT_TIMEOUT = 30000;
-
-
- public AlibabaCloudSnitch() throws MalformedURLException, IOException
- {
- String response = alibabaApiCall(ZONE_NAME_QUERY_URL);
- String[] splits = response.split("/");
- String az = splits[splits.length - 1];
-
- // Split "us-central1-a" or "asia-east1-a" into "us-central1"/"a" and "asia-east1"/"a".
- splits = az.split("-");
- ecsZone = splits[splits.length - 1];
-
- int lastRegionIndex = az.lastIndexOf("-");
- ecsRegion = az.substring(0, lastRegionIndex);
+ private static final String DEFAULT_METADATA_SERVICE_URL = "http://100.100.100.200";
+ private static final String ZONE_NAME_QUERY_URL = "/latest/meta-data/zone-id";
- String datacenterSuffix = (new SnitchProperties()).get("dc_suffix", "");
- ecsRegion = ecsRegion.concat(datacenterSuffix);
- logger.info("AlibabaSnitch using region: {}, zone: {}.", ecsRegion, ecsZone);
-
- }
-
- String alibabaApiCall(String url) throws ConfigurationException, IOException, SocketTimeoutException
+ public AlibabaCloudSnitch() throws IOException
{
- // Populate the region and zone by introspection, fail if 404 on metadata
- HttpURLConnection conn = (HttpURLConnection) new URL(url).openConnection();
- DataInputStream d = null;
- try
- {
- conn.setConnectTimeout(HTTP_CONNECT_TIMEOUT);
- conn.setRequestMethod("GET");
-
- int code = conn.getResponseCode();
- if (code != HttpURLConnection.HTTP_OK)
- throw new ConfigurationException("AlibabaSnitch was unable to execute the API call. Not an ecs node? and the returun code is " + code);
-
- // Read the information. I wish I could say (String) conn.getContent() here...
- int cl = conn.getContentLength();
- byte[] b = new byte[cl];
- d = new DataInputStream((FilterInputStream) conn.getContent());
- d.readFully(b);
- return new String(b, StandardCharsets.UTF_8);
- }
- catch (SocketTimeoutException e)
- {
- throw new SocketTimeoutException("Timeout occurred reading a response from the Alibaba ECS metadata");
- }
- finally
- {
- FileUtils.close(d);
- conn.disconnect();
- }
+ this(new SnitchProperties());
}
-
- @Override
- public String getRack(InetAddressAndPort endpoint)
+
+ public AlibabaCloudSnitch(SnitchProperties properties) throws IOException
{
- if (endpoint.equals(FBUtilities.getBroadcastAddressAndPort()))
- return ecsZone;
- EndpointState state = Gossiper.instance.getEndpointStateForEndpoint(endpoint);
- if (state == null || state.getApplicationState(ApplicationState.RACK) == null)
- {
- if (savedEndpoints == null)
- savedEndpoints = SystemKeyspace.loadDcRackInfo();
- if (savedEndpoints.containsKey(endpoint))
- return savedEndpoints.get(endpoint).get("rack");
- return DEFAULT_RACK;
- }
- return state.getApplicationState(ApplicationState.RACK).value;
-
+ this(properties, new DefaultCloudMetadataServiceConnector(properties.get(METADATA_URL_PROPERTY,
Review Comment:
See above comment about why it is split into two properties.
Once we implement "DefaultCloudServiceSnitch", we can do it like 1 address with whole endpoint.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1247586175
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,39 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+
+ public static class DefaultCloudMetadataServiceConnector extends AbstractCloudMetadataServiceConnector
+ {
+ protected DefaultCloudMetadataServiceConnector(String metadataServiceUrl)
Review Comment:
It is metadata service address only because that is variable we configure in mocking tests. See `Ec2V2ConnectorMockingTest` in trunk.
So the idea is that we change just the service url but not the endpoints.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257896450
##########
src/java/org/apache/cassandra/locator/DefaultCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.locator;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
+
+/**
+ * Default cloud-based metadata service snitch which parses datacenter and rack of a node by
+ * executing GET query for URL configured in cassandra-rackdc.properties under
+ * key {@link AbstractCloudMetadataServiceConnector#METADATA_URL_PROPERTY}. It expects HTTP response code 200.
+ * It is not passing any headers to GET request.
+ * <p>
+ * The response will be parsed like following - if the response body is, for example "us-central1-a", then
+ * the datacenter will be "us-central1" and rack will be "a". There is value of "dc_suffix" key in
+ * cassandra-rackdc.properties appended to datacenter.
+ */
+public class DefaultCloudMetadataServiceSnitch extends AbstractCloudMetadataServiceSnitch
Review Comment:
Yes, I believe it needs to be public. Keep in mind that we might reference this snitch in `cassandra.yaml` so we would be able to use this snitch for all cloud deployments when the response is parsed in a very opinionated way, for free, out of the box.
If this was not public, then it is not able to be constructed as making it package protected would throw:
org.apache.cassandra.exceptions.ConfigurationException: Default constructor for snitch class
'org.apache.cassandra.locator.MySnitch' is inaccessible.
at org.apache.cassandra.utils.FBUtilities.construct(FBUtilities.java:762)
at org.apache.cassandra.utils.FBUtilities.construct(FBUtilities.java:751)
at org.apache.cassandra.config.DatabaseDescriptor.createEndpointSnitch(DatabaseDescriptor.java:1491)
at org.apache.cassandra.config.DatabaseDescriptor.applySnitch(DatabaseDescriptor.java:1338)
at org.apache.cassandra.config.DatabaseDescriptor.applyAll(DatabaseDescriptor.java:435)
at org.apache.cassandra.config.DatabaseDescriptor.daemonInitialization(DatabaseDescriptor.java:244)
at org.apache.cassandra.config.DatabaseDescriptor.daemonInitialization(DatabaseDescriptor.java:228)
at org.apache.cassandra.service.CassandraDaemon.applyConfig(CassandraDaemon.java:819)
at org.apache.cassandra.service.CassandraDaemon.activate(CassandraDaemon.java:762)
at org.apache.cassandra.service.CassandraDaemon.main(CassandraDaemon.java:890)
With snitch like this (same holds for the DefaultCloudMetadataServiceSnitch)
class MySnitch extends AbstractNetworkTopologySnitch
{
@Override
public String getRack(InetAddressAndPort endpoint)
{
return "r1";
}
@Override
public String getDatacenter(InetAddressAndPort endpoint)
{
return "dc1";
}
}
We can not make it final either because `AlibabaCloudSnitch` is extending it.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257905441
##########
src/java/org/apache/cassandra/locator/Ec2MetadataServiceConnector.java:
##########
@@ -45,7 +45,7 @@ abstract class Ec2MetadataServiceConnector extends AbstractCloudMetadataServiceC
Ec2MetadataServiceConnector(SnitchProperties properties)
{
- super(properties.get(EC2_METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL));
+ super(properties.get(EC2_METADATA_URL_PROPERTY, properties.get(METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL)));
Review Comment:
Good question.
`METADATA_URL_PROPERTY = "metadata_url"`
and
`EC2_METADATA_URL_PROPERTY = "ec2_metadata_url"`.
The latter was introduced when we were doing IDMSv2 for AWS but with this PR in place, I wanted to be backward compatible.
As `ec2_metadata_url` key is in 3.0 -> 4.1 but it would not be in trunk, then upgrading to trunk would fail because that key would not be there anymore.
On the other hand, if one deploys it to trunk for the first time, it can set the url like "metadata_url" and I like the idea that metadata_url property would be same for all cloud snitches.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1258181130
##########
src/java/org/apache/cassandra/locator/Ec2MetadataServiceConnector.java:
##########
@@ -45,7 +45,7 @@ abstract class Ec2MetadataServiceConnector extends AbstractCloudMetadataServiceC
Ec2MetadataServiceConnector(SnitchProperties properties)
{
- super(properties.get(EC2_METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL));
+ super(properties.get(EC2_METADATA_URL_PROPERTY, properties.get(METADATA_URL_PROPERTY, DEFAULT_EC2_METADATA_URL)));
Review Comment:
yeah ... true, hm.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257909237
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,41 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+
+ public static class DefaultCloudMetadataServiceConnector extends AbstractCloudMetadataServiceConnector
+ {
+ protected DefaultCloudMetadataServiceConnector(String metadataServiceUrl)
+ {
+ super(metadataServiceUrl);
+ }
+ }
+
protected final String metadataServiceUrl;
+ @VisibleForTesting // for mockito
+ protected String getMetadataServiceUrl()
+ {
+ return metadataServiceUrl;
+ }
+
protected AbstractCloudMetadataServiceConnector(String metadataServiceUrl)
{
+ Preconditions.checkNotNull(metadataServiceUrl, METADATA_URL_PROPERTY + " for a connector can not be null");
this.metadataServiceUrl = metadataServiceUrl;
}
public String apiCall(String query) throws IOException
{
- return apiCall(metadataServiceUrl, query, 200);
+ return apiCall(getMetadataServiceUrl(), query, 200);
Review Comment:
This is here due to tests. I had to mock what that method would return, this can not be done on a field.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257896450
##########
src/java/org/apache/cassandra/locator/DefaultCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.locator;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
+
+/**
+ * Default cloud-based metadata service snitch which parses datacenter and rack of a node by
+ * executing GET query for URL configured in cassandra-rackdc.properties under
+ * key {@link AbstractCloudMetadataServiceConnector#METADATA_URL_PROPERTY}. It expects HTTP response code 200.
+ * It is not passing any headers to GET request.
+ * <p>
+ * The response will be parsed like following - if the response body is, for example "us-central1-a", then
+ * the datacenter will be "us-central1" and rack will be "a". There is value of "dc_suffix" key in
+ * cassandra-rackdc.properties appended to datacenter.
+ */
+public class DefaultCloudMetadataServiceSnitch extends AbstractCloudMetadataServiceSnitch
Review Comment:
Yes, I believe it needs to be public. Keep in mind that we might reference this snitch in `cassandra.yaml` so we would be able to use this snitch for all cloud deployments when the response is parsed in a very opinionated way, for free, out of the box.
If this was not public, then it is not able to be constructed as making it package protected would throw:
org.apache.cassandra.exceptions.ConfigurationException: Default constructor for snitch class
'org.apache.cassandra.locator.MySnitch' is inaccessible.
at org.apache.cassandra.utils.FBUtilities.construct(FBUtilities.java:762)
at org.apache.cassandra.utils.FBUtilities.construct(FBUtilities.java:751)
at org.apache.cassandra.config.DatabaseDescriptor.createEndpointSnitch(DatabaseDescriptor.java:1491)
at org.apache.cassandra.config.DatabaseDescriptor.applySnitch(DatabaseDescriptor.java:1338)
at org.apache.cassandra.config.DatabaseDescriptor.applyAll(DatabaseDescriptor.java:435)
at org.apache.cassandra.config.DatabaseDescriptor.daemonInitialization(DatabaseDescriptor.java:244)
at org.apache.cassandra.config.DatabaseDescriptor.daemonInitialization(DatabaseDescriptor.java:228)
at org.apache.cassandra.service.CassandraDaemon.applyConfig(CassandraDaemon.java:819)
at org.apache.cassandra.service.CassandraDaemon.activate(CassandraDaemon.java:762)
at org.apache.cassandra.service.CassandraDaemon.main(CassandraDaemon.java:890)
With snitch like this (same holds for the DefaultCloudMetadataServiceSnitch)
class MySnitch extends AbstractNetworkTopologySnitch
{
@Override
public String getRack(InetAddressAndPort endpoint)
{
return "r1";
}
@Override
public String getDatacenter(InetAddressAndPort endpoint)
{
return "dc1";
}
}
This does not help either
class MySnitch extends AbstractNetworkTopologySnitch
{
public MySnitch() {}
....
}
We can not make it final either because `AlibabaCloudSnitch` is extending it.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257909237
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,41 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+
+ public static class DefaultCloudMetadataServiceConnector extends AbstractCloudMetadataServiceConnector
+ {
+ protected DefaultCloudMetadataServiceConnector(String metadataServiceUrl)
+ {
+ super(metadataServiceUrl);
+ }
+ }
+
protected final String metadataServiceUrl;
+ @VisibleForTesting // for mockito
+ protected String getMetadataServiceUrl()
+ {
+ return metadataServiceUrl;
+ }
+
protected AbstractCloudMetadataServiceConnector(String metadataServiceUrl)
{
+ Preconditions.checkNotNull(metadataServiceUrl, METADATA_URL_PROPERTY + " for a connector can not be null");
this.metadataServiceUrl = metadataServiceUrl;
}
public String apiCall(String query) throws IOException
{
- return apiCall(metadataServiceUrl, query, 200);
+ return apiCall(getMetadataServiceUrl(), query, 200);
Review Comment:
This is here due to tests. I had to mock what would that method return, this can not be done on a field.
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1258175874
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,41 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
Review Comment:
ah, yes, 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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1247382945
##########
src/java/org/apache/cassandra/locator/AlibabaCloudSnitch.java:
##########
@@ -17,130 +17,47 @@
*/
package org.apache.cassandra.locator;
-import java.io.DataInputStream;
-import java.io.FilterInputStream;
import java.io.IOException;
-import java.net.HttpURLConnection;
-import java.net.MalformedURLException;
-import java.net.SocketTimeoutException;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.util.Map;
-import org.apache.cassandra.db.SystemKeyspace;
-import org.apache.cassandra.exceptions.ConfigurationException;
-import org.apache.cassandra.gms.ApplicationState;
-import org.apache.cassandra.gms.EndpointState;
-import org.apache.cassandra.gms.Gossiper;
-import org.apache.cassandra.io.util.FileUtils;
-import org.apache.cassandra.utils.FBUtilities;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
/**
- * A snitch that assumes an ECS region is a DC and an ECS availability_zone
- * is a rack. This information is available in the config for the node. the
- * format of the zone-id is like :cn-hangzhou-a where cn means china, hangzhou
- * means the hangzhou region, a means the az id. We use cn-hangzhou as the dc,
- * and f as the zone-id.
+ * A snitch that assumes an ECS region is a DC and an ECS availability_zone
+ * is a rack. This information is available in the config for the node. the
+ * format of the zone-id is like :cn-hangzhou-a where cn means china, hangzhou
+ * means the hangzhou region, a means the az id. We use cn-hangzhou as the dc,
+ * and f as the zone-id.
*/
-public class AlibabaCloudSnitch extends AbstractNetworkTopologySnitch
+public class AlibabaCloudSnitch extends AbstractCloudMetadataServiceSnitch
{
- protected static final Logger logger = LoggerFactory.getLogger(AlibabaCloudSnitch.class);
- protected static final String ZONE_NAME_QUERY_URL = "http://100.100.100.200/latest/meta-data/zone-id";
- private static final String DEFAULT_DC = "UNKNOWN-DC";
- private static final String DEFAULT_RACK = "UNKNOWN-RACK";
- private Map<InetAddressAndPort, Map<String, String>> savedEndpoints;
- protected String ecsZone;
- protected String ecsRegion;
-
- private static final int HTTP_CONNECT_TIMEOUT = 30000;
-
-
- public AlibabaCloudSnitch() throws MalformedURLException, IOException
- {
- String response = alibabaApiCall(ZONE_NAME_QUERY_URL);
- String[] splits = response.split("/");
- String az = splits[splits.length - 1];
-
- // Split "us-central1-a" or "asia-east1-a" into "us-central1"/"a" and "asia-east1"/"a".
- splits = az.split("-");
- ecsZone = splits[splits.length - 1];
-
- int lastRegionIndex = az.lastIndexOf("-");
- ecsRegion = az.substring(0, lastRegionIndex);
+ private static final String DEFAULT_METADATA_SERVICE_URL = "http://100.100.100.200";
+ private static final String ZONE_NAME_QUERY_URL = "/latest/meta-data/zone-id";
- String datacenterSuffix = (new SnitchProperties()).get("dc_suffix", "");
- ecsRegion = ecsRegion.concat(datacenterSuffix);
- logger.info("AlibabaSnitch using region: {}, zone: {}.", ecsRegion, ecsZone);
-
- }
-
- String alibabaApiCall(String url) throws ConfigurationException, IOException, SocketTimeoutException
+ public AlibabaCloudSnitch() throws IOException
{
- // Populate the region and zone by introspection, fail if 404 on metadata
- HttpURLConnection conn = (HttpURLConnection) new URL(url).openConnection();
- DataInputStream d = null;
- try
- {
- conn.setConnectTimeout(HTTP_CONNECT_TIMEOUT);
- conn.setRequestMethod("GET");
-
- int code = conn.getResponseCode();
- if (code != HttpURLConnection.HTTP_OK)
- throw new ConfigurationException("AlibabaSnitch was unable to execute the API call. Not an ecs node? and the returun code is " + code);
-
- // Read the information. I wish I could say (String) conn.getContent() here...
- int cl = conn.getContentLength();
- byte[] b = new byte[cl];
- d = new DataInputStream((FilterInputStream) conn.getContent());
- d.readFully(b);
- return new String(b, StandardCharsets.UTF_8);
- }
- catch (SocketTimeoutException e)
- {
- throw new SocketTimeoutException("Timeout occurred reading a response from the Alibaba ECS metadata");
- }
- finally
- {
- FileUtils.close(d);
- conn.disconnect();
- }
+ this(new SnitchProperties());
}
-
- @Override
- public String getRack(InetAddressAndPort endpoint)
+
+ public AlibabaCloudSnitch(SnitchProperties properties) throws IOException
{
- if (endpoint.equals(FBUtilities.getBroadcastAddressAndPort()))
- return ecsZone;
- EndpointState state = Gossiper.instance.getEndpointStateForEndpoint(endpoint);
- if (state == null || state.getApplicationState(ApplicationState.RACK) == null)
- {
- if (savedEndpoints == null)
- savedEndpoints = SystemKeyspace.loadDcRackInfo();
- if (savedEndpoints.containsKey(endpoint))
- return savedEndpoints.get(endpoint).get("rack");
- return DEFAULT_RACK;
- }
- return state.getApplicationState(ApplicationState.RACK).value;
-
+ this(properties, new DefaultCloudMetadataServiceConnector(properties.get(METADATA_URL_PROPERTY,
Review Comment:
what about merge METADATA_URL_PROPERTY and ZONE_NAME_QUERY_URL together as before not just split them into two Strings and make them configurable.
or make ZONE_NAME_QUERY_URL configurable through Snitchpropetry ? (In this way I may suggest changing the name to ZONE_NAME_QUERY_URL_PATH)
##########
src/java/org/apache/cassandra/locator/AlibabaCloudSnitch.java:
##########
@@ -17,130 +17,47 @@
*/
package org.apache.cassandra.locator;
-import java.io.DataInputStream;
-import java.io.FilterInputStream;
import java.io.IOException;
-import java.net.HttpURLConnection;
-import java.net.MalformedURLException;
-import java.net.SocketTimeoutException;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.util.Map;
-import org.apache.cassandra.db.SystemKeyspace;
-import org.apache.cassandra.exceptions.ConfigurationException;
-import org.apache.cassandra.gms.ApplicationState;
-import org.apache.cassandra.gms.EndpointState;
-import org.apache.cassandra.gms.Gossiper;
-import org.apache.cassandra.io.util.FileUtils;
-import org.apache.cassandra.utils.FBUtilities;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
/**
- * A snitch that assumes an ECS region is a DC and an ECS availability_zone
- * is a rack. This information is available in the config for the node. the
- * format of the zone-id is like :cn-hangzhou-a where cn means china, hangzhou
- * means the hangzhou region, a means the az id. We use cn-hangzhou as the dc,
- * and f as the zone-id.
+ * A snitch that assumes an ECS region is a DC and an ECS availability_zone
+ * is a rack. This information is available in the config for the node. the
+ * format of the zone-id is like :cn-hangzhou-a where cn means china, hangzhou
+ * means the hangzhou region, a means the az id. We use cn-hangzhou as the dc,
+ * and f as the zone-id.
Review Comment:
sorry I make a misstake at the first time push this pr, this should be "a as the zone-id" not f.
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,39 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+
+ public static class DefaultCloudMetadataServiceConnector extends AbstractCloudMetadataServiceConnector
+ {
+ protected DefaultCloudMetadataServiceConnector(String metadataServiceUrl)
Review Comment:
as said before , what about make this metaserviceUrl with the full url path ,not only the url address.
Or make both zone path configurable which may be more flexible.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257923363
##########
src/java/org/apache/cassandra/locator/DefaultCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.locator;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
+
+/**
+ * Default cloud-based metadata service snitch which parses datacenter and rack of a node by
+ * executing GET query for URL configured in cassandra-rackdc.properties under
+ * key {@link AbstractCloudMetadataServiceConnector#METADATA_URL_PROPERTY}. It expects HTTP response code 200.
+ * It is not passing any headers to GET request.
+ * <p>
+ * The response will be parsed like following - if the response body is, for example "us-central1-a", then
+ * the datacenter will be "us-central1" and rack will be "a". There is value of "dc_suffix" key in
+ * cassandra-rackdc.properties appended to datacenter.
+ */
+public class DefaultCloudMetadataServiceSnitch extends AbstractCloudMetadataServiceSnitch
+{
Review Comment:
works for me.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259512217
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -61,6 +80,7 @@ public String apiCall(String url,
conn = (HttpURLConnection) new URL(url + query).openConnection();
extraHeaders.forEach(conn::setRequestProperty);
conn.setRequestMethod(method);
+ conn.setConnectTimeout(30000);
Review Comment:
nobody is ever going to configure it, we dont need this
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259329612
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.locator;
+
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.db.SystemKeyspace;
+import org.apache.cassandra.gms.ApplicationState;
+import org.apache.cassandra.gms.EndpointState;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.utils.FBUtilities;
+import org.apache.cassandra.utils.Pair;
+
+import static java.lang.String.format;
+
+abstract class AbstractCloudMetadataServiceSnitch extends AbstractNetworkTopologySnitch
+{
+ static final Logger logger = LoggerFactory.getLogger(AbstractCloudMetadataServiceSnitch.class);
+
+ static final String DEFAULT_DC = "UNKNOWN-DC";
+ static final String DEFAULT_RACK = "UNKNOWN-RACK";
+
+ protected final AbstractCloudMetadataServiceConnector connector;
+ protected final SnitchProperties snitchProperties;
+
+ private final String localRack;
+ private final String localDc;
+
+ private Map<InetAddressAndPort, Map<String, String>> savedEndpoints;
+
+ public AbstractCloudMetadataServiceSnitch(AbstractCloudMetadataServiceConnector connector,
+ SnitchProperties snitchProperties,
+ Pair<String, String> dcAndRack)
+ {
+ this.connector = connector;
+ this.snitchProperties = snitchProperties;
+ this.localDc = dcAndRack.left;
+ this.localRack = dcAndRack.right;
+
+ logger.info(format("%s using datacenter: %s, rack: %s, connector: %s, properties: %s",
+ getClass().getName(), getLocalDc(), getLocalRack(), connector, snitchProperties));
+ }
+
+ @Override
+ public String getLocalRack()
+ {
+ return localRack;
+ }
+
+ public String getLocalDc()
Review Comment:
I don't know if it is intentional, but there is a `getLocalDatacenter()` method in the `IEndpointSnitch` interface with some default 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: 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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257922508
##########
src/java/org/apache/cassandra/locator/DefaultCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.locator;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+
+import org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.DefaultCloudMetadataServiceConnector;
+
+import static org.apache.cassandra.locator.AbstractCloudMetadataServiceConnector.METADATA_URL_PROPERTY;
+
+/**
+ * Default cloud-based metadata service snitch which parses datacenter and rack of a node by
+ * executing GET query for URL configured in cassandra-rackdc.properties under
+ * key {@link AbstractCloudMetadataServiceConnector#METADATA_URL_PROPERTY}. It expects HTTP response code 200.
+ * It is not passing any headers to GET request.
+ * <p>
+ * The response will be parsed like following - if the response body is, for example "us-central1-a", then
+ * the datacenter will be "us-central1" and rack will be "a". There is value of "dc_suffix" key in
+ * cassandra-rackdc.properties appended to datacenter.
+ */
+public class DefaultCloudMetadataServiceSnitch extends AbstractCloudMetadataServiceSnitch
+{
+ public DefaultCloudMetadataServiceSnitch() throws IOException
+ {
+ this(new SnitchProperties());
+ }
+
+ public DefaultCloudMetadataServiceSnitch(SnitchProperties properties) throws IOException
Review Comment:
I don't this that is really necessary.
I can do this though.
public DefaultCloudMetadataServiceSnitch(String url) throws IOException
{
this(new SnitchProperties(Pair.create(METADATA_URL_PROPERTY, url)));
}
If you look how the response is parsed etc, we also need dc suffix propagated and in case like I did it it would not be parsed from property file.
I can also do this
public DefaultCloudMetadataServiceSnitch(SnitchProperties properties, String url) throws IOException
{
this(properties, new DefaultCloudMetadataServiceConnector(url));
}
But why would you do that if it is going to be taken from `properties` anyway as we need them?
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259478109
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -61,6 +80,7 @@ public String apiCall(String url,
conn = (HttpURLConnection) new URL(url + query).openConnection();
extraHeaders.forEach(conn::setRequestProperty);
conn.setRequestMethod(method);
+ conn.setConnectTimeout(30000);
Review Comment:
the original codes are all hard code, I think we can make this property in snitch with default value of 30s , also I don't insist on this one ,same with jacek.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1257907797
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -25,22 +25,41 @@
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
Review Comment:
I dont think that is actually necessary. It is not like this property is a _system property_. This is just a key of a property in cassandra-rackdc.properties.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259411595
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceSnitch.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.locator;
+
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.db.SystemKeyspace;
+import org.apache.cassandra.gms.ApplicationState;
+import org.apache.cassandra.gms.EndpointState;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.utils.FBUtilities;
+import org.apache.cassandra.utils.Pair;
+
+import static java.lang.String.format;
+
+abstract class AbstractCloudMetadataServiceSnitch extends AbstractNetworkTopologySnitch
+{
+ static final Logger logger = LoggerFactory.getLogger(AbstractCloudMetadataServiceSnitch.class);
+
+ static final String DEFAULT_DC = "UNKNOWN-DC";
+ static final String DEFAULT_RACK = "UNKNOWN-RACK";
+
+ protected final AbstractCloudMetadataServiceConnector connector;
+ protected final SnitchProperties snitchProperties;
+
+ private final String localRack;
+ private final String localDc;
+
+ private Map<InetAddressAndPort, Map<String, String>> savedEndpoints;
+
+ public AbstractCloudMetadataServiceSnitch(AbstractCloudMetadataServiceConnector connector,
+ SnitchProperties snitchProperties,
+ Pair<String, String> dcAndRack)
+ {
+ this.connector = connector;
+ this.snitchProperties = snitchProperties;
+ this.localDc = dcAndRack.left;
+ this.localRack = dcAndRack.right;
+
+ logger.info(format("%s using datacenter: %s, rack: %s, connector: %s, properties: %s",
+ getClass().getName(), getLocalDc(), getLocalRack(), connector, snitchProperties));
+ }
+
+ @Override
+ public String getLocalRack()
+ {
+ return localRack;
+ }
+
+ public String getLocalDc()
Review Comment:
great catch.
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2458: CASSANDRA-18438
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2458:
URL: https://github.com/apache/cassandra/pull/2458#discussion_r1259500502
##########
src/java/org/apache/cassandra/locator/AbstractCloudMetadataServiceConnector.java:
##########
@@ -22,25 +22,55 @@
import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
+import java.net.MalformedURLException;
+import java.net.URISyntaxException;
import java.net.URL;
import java.util.Map;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
+import org.apache.cassandra.exceptions.ConfigurationException;
+
import static java.nio.charset.StandardCharsets.UTF_8;
abstract class AbstractCloudMetadataServiceConnector
{
+ static final String METADATA_URL_PROPERTY = "metadata_url";
+
+ public static class DefaultCloudMetadataServiceConnector extends AbstractCloudMetadataServiceConnector
+ {
+ protected DefaultCloudMetadataServiceConnector(String metadataServiceUrl)
+ {
+ super(metadataServiceUrl);
+ }
+ }
+
protected final String metadataServiceUrl;
+ @VisibleForTesting // for mockito
+ protected String getMetadataServiceUrl()
+ {
+ return metadataServiceUrl;
+ }
+
protected AbstractCloudMetadataServiceConnector(String metadataServiceUrl)
{
+ try
+ {
+ URL url = new URL(metadataServiceUrl);
+ url.toURI();
+ }
+ catch (MalformedURLException | IllegalArgumentException | URISyntaxException ex)
+ {
+ throw new ConfigurationException(String.format("URL you provided (%s) is not valid!", metadataServiceUrl), ex);
Review Comment:
Maybe something like:
Snitch metadata service URL '%s' is invalid. Please review snitch properties defined in the 'cassandra-rackdc.properties' configuration file.
--
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