Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate schema designer to filestoreapi part deux #3031

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

epugh
Copy link
Contributor

@epugh epugh commented Jan 14, 2025

https://issues.apache.org/jira/browse/SOLR-XXXXX

Description

Migrate Schema Designer calls from using Blob store to FileStore

Solution

Swap implementation methods.

Tests

So far, just using existing tests.

@epugh
Copy link
Contributor Author

epugh commented Jan 14, 2025

Progress so far @gerlowskija ....

@@ -534,7 +534,6 @@ public void refresh(String path) {
@SuppressWarnings({"rawtypes"})
List myFiles = list(path, s -> true);
for (Object f : l) {
// TODO: https://issues.apache.org/jira/browse/SOLR-15426
Copy link
Contributor Author

@epugh epugh Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madrob you opened up this JIRA, and I think this TODO is incorrect or otherwise overcome by events. Regardless, it doesn't appear connected to SOLR-15426... Can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was actually related to SOLR-15385, I must have made a copy/paste error here. Reading my comments and surface-level tracing the code suggests that this is still an issue.

@epugh epugh requested a review from gerlowskija January 14, 2025 15:17
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
}
String path =
"blob" + "/" + configSet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[0] Might be nice to make the "blob/" bit here and other similar path segments constants, that can be reused on both the storage and retrieval side of this helper code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely. I don't even think we should use "blob" prefix. Maybe "schema_designer" since this is for this tool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the format of this data is "javabin", lets name the file with that suffix.

@gerlowskija
Copy link
Contributor

Looks good to me so far. I notice this is still "Draft"; curious what else is remaining here?

Comment on lines +48 to +49
* <p>TODO: Is fetchMissing actually used? I don't see it being used, but the IDE doesn't flag it
* not being used!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lucene/Solr devs use a "nocommit" comment for something like this. Precommit will fail so we remember to address it. I also configure IntelliJ's "TODO" feature to consider "nocommit" and thus it shows in blood red (gets my attention)

throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
}
String path =
"blob" + "/" + configSet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely. I don't even think we should use "blob" prefix. Maybe "schema_designer" since this is for this tool.

throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
}
String path =
"blob" + "/" + configSet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the format of this data is "javabin", lets name the file with that suffix.

}
},
true);
} catch (java.io.FileNotFoundException e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why fully qualified

Comment on lines +534 to +537
byte[] bytes = is.readAllBytes();
if (bytes.length > 0) {
docs = (List<SolrInputDocument>) Utils.fromJavabin(bytes);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a little comment // TODO stream it; no byte array

} finally {
httpPost.releaseConnection();
}
FileStoreAPI.MetaData meta = ClusterFileStore._createJsonMetaData(bytes, null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the meta necessary in general for the FileStoreAPI -- 2 step? Sad if so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants