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

SOLR-16038: Rename PackageStore as Filestore #1908

Merged

Conversation

janhoy
Copy link
Contributor

@janhoy janhoy commented Sep 8, 2023

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

I did NOT rename the /packagestore name in zookeeper and related constants, as it would involve migrating/renaming..

Did not create a CHANGES entry as this is an internal code-change only

@janhoy janhoy requested review from noblepaul and madrob September 8, 2023 18:48
@janhoy
Copy link
Contributor Author

janhoy commented Sep 19, 2023

@noblepaul do you want to have a look at this?

@epugh
Copy link
Contributor

epugh commented Oct 10, 2023

I'd love to see the renames happen... I think it's more confusing to have one name in the code and another in the zookeeper. Could we just allow both? I.e keep the old and the new name? for a while? like we supported "schema" for a while after "schema.xml" was the name in Zookeeper?

@epugh
Copy link
Contributor

epugh commented Oct 10, 2023

Also, I bumped into the fact that you can't use the PackageStore (soon to be File Store) if you don't pass in -Denable.packages=true. Should we change that to enable.filestore? Or, maybe, we just want to say that packages are different than files. So you can use the filestore apis normally but not packages... HOnestly, in a lot of ways, i want these various "enable" switches to be true by default.. cause I don't know i want a package until AFTER i've deployed my serach engine ;-)

@janhoy
Copy link
Contributor Author

janhoy commented Oct 10, 2023

Packages are disabled by default since they are beta quality and potentially a serious security hole. Need much more use, feeback and bugfixes before we can flip the default. In general I'm sceptical to allow any form of live download of code at runtime. In the day and age of containers there should be means of adding packages during container image build and then deploy code to running cluster through the normal deployment routines.

@epugh
Copy link
Contributor

epugh commented Oct 11, 2023

Two thoughts on your last message:

  1. should we label it experimental so we can make changes without worrying about back compatiblity if we think that there is still a lot of work to be done? For example, this ticket is literally being hampered by the back compat desire.

  2. I think there are two competing philosophies, the "build a static image and deploy it with everything" and the "I want to change things at runtime". We kind of support both in various ways across Solr... So I can see the "live download of code" being an issue... But I can also imagine a "We deploy a live package every few hours with our latest model that does X" and wouldn't want to rebuild everything..

Both comments are probably getting out of scope of this PR ;-).

@janhoy
Copy link
Contributor Author

janhoy commented Oct 11, 2023

Yea, probably concentrate on the rename for now 😉

@janhoy janhoy requested review from epugh and gerlowskija November 3, 2023 09:01
@janhoy
Copy link
Contributor Author

janhoy commented Nov 3, 2023

Added a few more reviewers and brought up to date with main. Again, this is an internal java-level change and the API is experimental so no back compat requirements.

As followup issues, we could perhaps investigate how the filestore could be utilized by other parts of Solr to store large models etc, disconnected from the package feature.

@epugh
Copy link
Contributor

epugh commented Nov 3, 2023

I checked out the branch and did a search for "PackageStore" and found a number of places it is still mentioned... DistribFileStore.java has a:

  /** This is where al the files in the package store are listed */
  static final String ZK_PACKAGESTORE = "/packagestore";

I wish it was ZK_FILESTORE and /filestore. Unless we had a story about a "packagestore" being some sort of specialization of the generic "filestore".... but i don't think jthat is the case.

That constant shows up a few times.

Likewise FileStoreAPI has:

    static final String TMP_ZK_NODE = "/packageStoreWriteInProgress";

And TestDistribFileStore has testPackageStoreManagement method...

- Rename temp zk node
- Fix exception messages
@janhoy
Copy link
Contributor Author

janhoy commented Nov 3, 2023

I wish it was ZK_FILESTORE and /filestore

As mentioned in the PR description I left the ZK node name as-is.

I fixed the TMP_ZK_NODE and two exception messages.

@janhoy
Copy link
Contributor Author

janhoy commented Nov 7, 2023

@epugh I also renamed the test method you mentioned. Think this is good to go now?

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

LGTM. This will be a great building block for more things.

@janhoy janhoy merged commit b17b17d into apache:main Nov 7, 2023
2 checks passed
janhoy added a commit that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants