dsmiley commented on code in PR #3031:
URL: https://github.com/apache/solr/pull/3031#discussion_r2265050567
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerConfigSetHelper.java:
##########
@@ -466,44 +469,62 @@ protected void validateTypeChange(String configSet,
SchemaField field, FieldType
SolrException.ErrorCode.BAD_REQUEST,
"Cannot change type of the _version_ field; it must be a plong.");
}
- List<SolrInputDocument> docs = getStoredSampleDocs(configSet);
+ List<SolrInputDocument> docs = retrieveSampleDocs(configSet);
if (!docs.isEmpty()) {
schemaSuggester.validateTypeChange(field, toType, docs);
}
}
+ String getSampleDocsPathFromConfigSet(String configSet) {
+ String path = "schemadesigner" + "/" + configSet + "_sampledocs.javabin";
+ return path;
+ }
+
void deleteStoredSampleDocs(String configSet) {
- try {
- cloudClient().deleteByQuery(BLOB_STORE_ID, "id:" + configSet +
"_sample/*", 10);
- } catch (IOException | SolrServerException | SolrException exc) {
- final String excStr = exc.toString();
- log.warn("Failed to delete sample docs from blob store for {} due to:
{}", configSet, excStr);
- }
+ String path = getSampleDocsPathFromConfigSet(configSet);
+ // why do I have to do this in two stages?
+ DistribFileStore.deleteZKFileEntry(cc.getZkController().getZkClient(),
path);
+ cc.getFileStore().delete(path);
}
+ // I don't like this guy just hanging out here to support retrieveSampleDocs.
+ List<SolrInputDocument> docs = Collections.emptyList();
+
@SuppressWarnings("unchecked")
- List<SolrInputDocument> getStoredSampleDocs(final String configSet) throws
IOException {
- var request = new GenericSolrRequest(SolrRequest.METHOD.GET, "/blob/" +
configSet + "_sample");
- request.setRequiresCollection(true);
- request.setResponseParser(new InputStreamResponseParser("filestream"));
- InputStream inputStream = null;
+ List<SolrInputDocument> retrieveSampleDocs(final String configSet) throws
IOException {
+
+ String path = getSampleDocsPathFromConfigSet(configSet);
+
try {
- var resp = request.process(cloudClient(), BLOB_STORE_ID).getResponse();
- inputStream = (InputStream) resp.get("stream");
- var bytes = inputStream.readAllBytes();
- if (bytes.length > 0) {
- return (List<SolrInputDocument>) Utils.fromJavabin(bytes);
- } else return Collections.emptyList();
- } catch (SolrServerException e) {
- throw new IOException("Failed to lookup stored docs for " + configSet +
" due to: " + e);
- } finally {
- IOUtils.closeQuietly(inputStream);
+ cc.getFileStore()
+ .get(
+ path,
+ entry -> {
+ try (InputStream is = entry.getInputStream()) {
+ docs = (List<SolrInputDocument>) Utils.fromJavabin(is);
+ } catch (IOException e) {
+ log.error("Error reading file content", e);
Review Comment:
logging instead of propagating the error? I'd think this is
exception-worthy.
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerConfigSetHelper.java:
##########
@@ -466,44 +469,62 @@ protected void validateTypeChange(String configSet,
SchemaField field, FieldType
SolrException.ErrorCode.BAD_REQUEST,
"Cannot change type of the _version_ field; it must be a plong.");
}
- List<SolrInputDocument> docs = getStoredSampleDocs(configSet);
+ List<SolrInputDocument> docs = retrieveSampleDocs(configSet);
if (!docs.isEmpty()) {
schemaSuggester.validateTypeChange(field, toType, docs);
}
}
+ String getSampleDocsPathFromConfigSet(String configSet) {
+ String path = "schemadesigner" + "/" + configSet + "_sampledocs.javabin";
+ return path;
+ }
+
void deleteStoredSampleDocs(String configSet) {
- try {
- cloudClient().deleteByQuery(BLOB_STORE_ID, "id:" + configSet +
"_sample/*", 10);
- } catch (IOException | SolrServerException | SolrException exc) {
- final String excStr = exc.toString();
- log.warn("Failed to delete sample docs from blob store for {} due to:
{}", configSet, excStr);
- }
+ String path = getSampleDocsPathFromConfigSet(configSet);
+ // why do I have to do this in two stages?
+ DistribFileStore.deleteZKFileEntry(cc.getZkController().getZkClient(),
path);
+ cc.getFileStore().delete(path);
}
+ // I don't like this guy just hanging out here to support retrieveSampleDocs.
+ List<SolrInputDocument> docs = Collections.emptyList();
Review Comment:
Nowadays use `List.of()`
##########
solr/core/src/java/org/apache/solr/filestore/FileStore.java:
##########
@@ -71,12 +80,12 @@ public interface FileStore {
/** Delete file from local file system */
void deleteLocal(String path);
- public class FileEntry {
+ class FileEntry {
Review Comment:
record?
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerConfigSetHelper.java:
##########
@@ -466,44 +469,62 @@ protected void validateTypeChange(String configSet,
SchemaField field, FieldType
SolrException.ErrorCode.BAD_REQUEST,
"Cannot change type of the _version_ field; it must be a plong.");
}
- List<SolrInputDocument> docs = getStoredSampleDocs(configSet);
+ List<SolrInputDocument> docs = retrieveSampleDocs(configSet);
if (!docs.isEmpty()) {
schemaSuggester.validateTypeChange(field, toType, docs);
}
}
+ String getSampleDocsPathFromConfigSet(String configSet) {
+ String path = "schemadesigner" + "/" + configSet + "_sampledocs.javabin";
+ return path;
Review Comment:
just one line please
##########
solr/core/src/test/org/apache/solr/uninverting/TestFieldCacheSortRandom.java:
##########
@@ -266,23 +266,26 @@ public int compare(BytesRef a, BytesRef b) {
}
// For both STRING and STRING_VAL types in Lucene 10+, the internal
sorting behavior
- // has changed and can return different types (String vs BytesRef)
depending on load conditions.
- // The manual BytesRef.compareTo() used in this test doesn't reliably
match the actual Lucene sort
+ // has changed and can return different types (String vs BytesRef)
depending on load
+ // conditions.
+ // The manual BytesRef.compareTo() used in this test doesn't reliably
match the actual Lucene
+ // sort
// order under all conditions.
Review Comment:
join lines
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java:
##########
@@ -87,7 +87,7 @@
public class SchemaDesignerAPI implements SchemaDesignerConstants {
private static final Set<String> excludeConfigSetNames =
- new HashSet<>(Arrays.asList(DEFAULT_CONFIGSET_NAME, BLOB_STORE_ID));
+ new HashSet<>(Arrays.asList(DEFAULT_CONFIGSET_NAME));
Review Comment:
This should use `Set.of` as it's declared as a constant and thus needs to be
immutable
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerConfigSetHelper.java:
##########
@@ -466,44 +469,62 @@ protected void validateTypeChange(String configSet,
SchemaField field, FieldType
SolrException.ErrorCode.BAD_REQUEST,
"Cannot change type of the _version_ field; it must be a plong.");
}
- List<SolrInputDocument> docs = getStoredSampleDocs(configSet);
+ List<SolrInputDocument> docs = retrieveSampleDocs(configSet);
if (!docs.isEmpty()) {
schemaSuggester.validateTypeChange(field, toType, docs);
}
}
+ String getSampleDocsPathFromConfigSet(String configSet) {
+ String path = "schemadesigner" + "/" + configSet + "_sampledocs.javabin";
+ return path;
+ }
+
void deleteStoredSampleDocs(String configSet) {
- try {
- cloudClient().deleteByQuery(BLOB_STORE_ID, "id:" + configSet +
"_sample/*", 10);
- } catch (IOException | SolrServerException | SolrException exc) {
- final String excStr = exc.toString();
- log.warn("Failed to delete sample docs from blob store for {} due to:
{}", configSet, excStr);
- }
+ String path = getSampleDocsPathFromConfigSet(configSet);
+ // why do I have to do this in two stages?
+ DistribFileStore.deleteZKFileEntry(cc.getZkController().getZkClient(),
path);
+ cc.getFileStore().delete(path);
}
+ // I don't like this guy just hanging out here to support retrieveSampleDocs.
Review Comment:
IMO comments of this nature should be TODO.
I don't like this guy either.
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerConfigSetHelper.java:
##########
@@ -466,44 +469,62 @@ protected void validateTypeChange(String configSet,
SchemaField field, FieldType
SolrException.ErrorCode.BAD_REQUEST,
"Cannot change type of the _version_ field; it must be a plong.");
}
- List<SolrInputDocument> docs = getStoredSampleDocs(configSet);
+ List<SolrInputDocument> docs = retrieveSampleDocs(configSet);
if (!docs.isEmpty()) {
schemaSuggester.validateTypeChange(field, toType, docs);
}
}
+ String getSampleDocsPathFromConfigSet(String configSet) {
+ String path = "schemadesigner" + "/" + configSet + "_sampledocs.javabin";
+ return path;
+ }
+
void deleteStoredSampleDocs(String configSet) {
- try {
- cloudClient().deleteByQuery(BLOB_STORE_ID, "id:" + configSet +
"_sample/*", 10);
- } catch (IOException | SolrServerException | SolrException exc) {
- final String excStr = exc.toString();
- log.warn("Failed to delete sample docs from blob store for {} due to:
{}", configSet, excStr);
- }
+ String path = getSampleDocsPathFromConfigSet(configSet);
+ // why do I have to do this in two stages?
+ DistribFileStore.deleteZKFileEntry(cc.getZkController().getZkClient(),
path);
+ cc.getFileStore().delete(path);
}
+ // I don't like this guy just hanging out here to support retrieveSampleDocs.
+ List<SolrInputDocument> docs = Collections.emptyList();
+
@SuppressWarnings("unchecked")
- List<SolrInputDocument> getStoredSampleDocs(final String configSet) throws
IOException {
- var request = new GenericSolrRequest(SolrRequest.METHOD.GET, "/blob/" +
configSet + "_sample");
- request.setRequiresCollection(true);
- request.setResponseParser(new InputStreamResponseParser("filestream"));
- InputStream inputStream = null;
+ List<SolrInputDocument> retrieveSampleDocs(final String configSet) throws
IOException {
+
+ String path = getSampleDocsPathFromConfigSet(configSet);
+
try {
- var resp = request.process(cloudClient(), BLOB_STORE_ID).getResponse();
- inputStream = (InputStream) resp.get("stream");
- var bytes = inputStream.readAllBytes();
- if (bytes.length > 0) {
- return (List<SolrInputDocument>) Utils.fromJavabin(bytes);
- } else return Collections.emptyList();
- } catch (SolrServerException e) {
- throw new IOException("Failed to lookup stored docs for " + configSet +
" due to: " + e);
- } finally {
- IOUtils.closeQuietly(inputStream);
+ cc.getFileStore()
+ .get(
+ path,
+ entry -> {
+ try (InputStream is = entry.getInputStream()) {
+ docs = (List<SolrInputDocument>) Utils.fromJavabin(is);
+ } catch (IOException e) {
+ log.error("Error reading file content", e);
+ }
+ },
+ true);
+ } catch (FileNotFoundException e) {
+ log.info("File at path {} not found.", path);
}
+
+ return docs != null ? docs : Collections.emptyList();
Review Comment:
`List.of()` again
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
