You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/07/13 18:09:06 UTC
[GitHub] [cassandra-sidecar] frankgh opened a new pull request, #34: CASSANDRASC-40 Fix search in list snapshot endpoint
frankgh opened a new pull request, #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34
This commit fixes test setup in SnapshotUtils. Because of the incorrect test setup
the execution is providing incorrect results. For example, assume the following path
/cassandra-test/data/ks/tbl/snapshots/test-snapshot
The test was configuring data directories as ["/cassandra-test/data"], but in a real
execution data directories is provided as ["/cassandra-test"]. This is causing the
endpoint to return incorrect values in the JSON payload.
Additionally, the response was providing the port for Cassandra and not the Sidecar
port.
--
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-sidecar] frankgh commented on pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
frankgh commented on PR #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34#issuecomment-1190768746
> Regarding the refactor, it still has the path extraction code, which I think is a dup and can be avoided. Please take a look at the alternative [yifan-c@9ce5c6d](https://github.com/yifan-c/cassandra-sidecar/commit/9ce5c6dbc2522fe4f43f07efbbb141b57e19a9cc)
Thanks for that, I spent some time exploring that implementation, but I ended up with more complexity in the code. In the end, I decided to step back for a minute and explore other options. I decided against the proposed approach because:
- the code becomes more complex
- some existing classes that depend on the existing API would need to be refactored
- the approach above was doing something very specific for the `listSnapshotDirectory` API
In the end, I realized we have the `snapshotDirectory` available to us in the `ListSnapshotFilesHandler`. That directory contains all the information required for us to build the response.
I ended up with a more simplified code, and moved the logic to the `SnapshotDirectory` class.
Please let me know what are your thoughts on this approach. I think it's actually looking much cleaner now, and it fixes a few assumptions as well, such as the data directory index in the response.
I have also marked the `ListSnapshotFilesResponse` class as `@Beta`. I suspect that we do not need that many elements in the response payload. The response is used in conjunction with the `StreamSSTableComponentHandler`, the handler does not need that information, so I think we should reconsider what fields are required in the response payload. But maybe let's have that as a follow up?
--
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-sidecar] frankgh closed pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
frankgh closed pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
URL: https://github.com/apache/cassandra-sidecar/pull/34
--
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-sidecar] yifan-c commented on a diff in pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34#discussion_r923829228
##########
src/main/java/org/apache/cassandra/sidecar/routes/ListSnapshotFilesHandler.java:
##########
@@ -146,6 +160,12 @@ private ListSnapshotFilesResponse buildResponse(String host,
return response;
}
+ private boolean isSecondaryIndexFile(Path path)
+ {
+ return path.getNameCount() >= 4 &&
+ path.getName(path.getNameCount() - 4).endsWith(SNAPSHOTS_DIR_NAME);
Review Comment:
I think it can still fail in some cases.
For example, `/cassandra/snapshot/ks/tbl/sstable-files`. The data directory is `/cassandra/snapshot`.
Since there is no scheme for the directory names, finding the root and counting from root can be more reliable.
--
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-sidecar] yifan-c commented on a diff in pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34#discussion_r921808410
##########
src/main/java/org/apache/cassandra/sidecar/routes/ListSnapshotFilesHandler.java:
##########
@@ -130,20 +121,18 @@ private ListSnapshotFilesResponse buildResponse(String host,
ListSnapshotFilesRequest request,
List<Pair<String, FileProps>> fileList)
{
- InstanceMetadata instanceMetadata = instancesConfig.instanceFromHost(host);
- int sidecarPort = instanceMetadata.port();
- Path dataDirPath = Paths.get(instanceMetadata.dataDirs().get(DATA_DIR_INDEX));
ListSnapshotFilesResponse response = new ListSnapshotFilesResponse();
String snapshotName = request.getSnapshotName();
+ int sidecarPort = configuration.getPort();
for (Pair<String, FileProps> file : fileList)
{
- Path pathFromDataDir = dataDirPath.relativize(Paths.get(file.getLeft()));
+ Path path = Paths.get(file.getLeft());
String keyspace = request.getKeyspace();
// table name might include a dash (-) with the table UUID so we always use it as part of the response
- String tableName = pathFromDataDir.getName(TABLE_NAME_SUBPATH_INDEX).toString();
- String fileName = pathFromDataDir.getName(FILE_NAME_SUBPATH_INDEX).toString();
+ String tableName = path.getName(path.getNameCount() - TABLE_NAME_SUBPATH_INDEX_FROM_END).toString();
Review Comment:
Counting from the end... does it work if includeSecondaryIndexFiles? It would add another level
##########
src/main/java/org/apache/cassandra/sidecar/routes/ListSnapshotFilesHandler.java:
##########
@@ -130,20 +121,18 @@ private ListSnapshotFilesResponse buildResponse(String host,
ListSnapshotFilesRequest request,
List<Pair<String, FileProps>> fileList)
{
- InstanceMetadata instanceMetadata = instancesConfig.instanceFromHost(host);
- int sidecarPort = instanceMetadata.port();
- Path dataDirPath = Paths.get(instanceMetadata.dataDirs().get(DATA_DIR_INDEX));
ListSnapshotFilesResponse response = new ListSnapshotFilesResponse();
String snapshotName = request.getSnapshotName();
+ int sidecarPort = configuration.getPort();
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34#discussion_r923933924
##########
common/src/main/java/org/apache/cassandra/sidecar/common/data/ListSnapshotFilesResponse.java:
##########
@@ -52,14 +52,14 @@ public List<FileInfo> getSnapshotFilesInfo()
*/
public static class FileInfo
{
- public final long size;
- public final String host;
- public final int port;
- public final int dataDirIndex;
- public final String snapshotName;
- public final String keySpaceName;
- public final String tableName;
- public final String fileName;
+ private final long size;
+ private final String host;
+ private final int port;
+ private final int dataDirIndex;
+ private final String snapshotName;
+ private final String keySpaceName;
+ private final String tableName;
+ private final String fileName;
Review Comment:
The change in this file does not seem to be necessary. Also, according to https://cassandra.apache.org/_/development/code_style.html#boilerplate, we prefer `public final` over getters.
--
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-sidecar] yifan-c commented on pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
yifan-c commented on PR #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34#issuecomment-1188660057
Regarding the refactor, it still has the path extraction code, which I think is a dup and can be avoided. Please take a look at the alternative https://github.com/yifan-c/cassandra-sidecar/commit/9ce5c6dbc2522fe4f43f07efbbb141b57e19a9cc
--
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-sidecar] frankgh commented on pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
frankgh commented on PR #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34#issuecomment-1263980386
closed via 07deb07d2e57a32c67643385403980a9a3c3fc95
--
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-sidecar] frankgh commented on a diff in pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34#discussion_r923930359
##########
src/main/java/org/apache/cassandra/sidecar/routes/ListSnapshotFilesHandler.java:
##########
@@ -146,6 +160,12 @@ private ListSnapshotFilesResponse buildResponse(String host,
return response;
}
+ private boolean isSecondaryIndexFile(Path path)
+ {
+ return path.getNameCount() >= 4 &&
+ path.getName(path.getNameCount() - 4).endsWith(SNAPSHOTS_DIR_NAME);
Review Comment:
discussed offline and we decided it's best to have `SnapshotPathBuilder#listSnapshotDirectory` return a List of `SnapshotFile` objects instead of returning a list of `Pair` objects.
--
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-sidecar] frankgh commented on a diff in pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34#discussion_r923680408
##########
src/main/java/org/apache/cassandra/sidecar/routes/ListSnapshotFilesHandler.java:
##########
@@ -130,20 +121,18 @@ private ListSnapshotFilesResponse buildResponse(String host,
ListSnapshotFilesRequest request,
List<Pair<String, FileProps>> fileList)
{
- InstanceMetadata instanceMetadata = instancesConfig.instanceFromHost(host);
- int sidecarPort = instanceMetadata.port();
- Path dataDirPath = Paths.get(instanceMetadata.dataDirs().get(DATA_DIR_INDEX));
ListSnapshotFilesResponse response = new ListSnapshotFilesResponse();
String snapshotName = request.getSnapshotName();
+ int sidecarPort = configuration.getPort();
for (Pair<String, FileProps> file : fileList)
{
- Path pathFromDataDir = dataDirPath.relativize(Paths.get(file.getLeft()));
+ Path path = Paths.get(file.getLeft());
String keyspace = request.getKeyspace();
// table name might include a dash (-) with the table UUID so we always use it as part of the response
- String tableName = pathFromDataDir.getName(TABLE_NAME_SUBPATH_INDEX).toString();
- String fileName = pathFromDataDir.getName(FILE_NAME_SUBPATH_INDEX).toString();
+ String tableName = path.getName(path.getNameCount() - TABLE_NAME_SUBPATH_INDEX_FROM_END).toString();
Review Comment:
good point. This will not work with secondary index files. I will address
--
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-sidecar] frankgh commented on a diff in pull request #34: CASSANDRASC-40 Fix search in list snapshot endpoint
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #34:
URL: https://github.com/apache/cassandra-sidecar/pull/34#discussion_r923830593
##########
src/main/java/org/apache/cassandra/sidecar/routes/ListSnapshotFilesHandler.java:
##########
@@ -146,6 +160,12 @@ private ListSnapshotFilesResponse buildResponse(String host,
return response;
}
+ private boolean isSecondaryIndexFile(Path path)
+ {
+ return path.getNameCount() >= 4 &&
+ path.getName(path.getNameCount() - 4).endsWith(SNAPSHOTS_DIR_NAME);
Review Comment:
I thought about that, but it's unlikely that this will occur since the table appends the UUID to the directory name. In any case, I am thinking we can also validate for the `keyspace`, by changing the signature to this:
```java
private boolean isSecondaryIndexFile(Path path, String keyspace)
{
return path.getNameCount() >= 6 &&
path.getName(path.getNameCount() - 6).endsWith(keyspace) &&
path.getName(path.getNameCount() - 4).endsWith(SNAPSHOTS_DIR_NAME);
}
```
What do you think?
--
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