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

feat(update): Add new repository update framework #1458

Merged
merged 42 commits into from
Oct 21, 2019

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented Oct 2, 2019

This PR replaces the Python-based repository update tool with one written in Scala, to improve performance.

Resolves #1451.

@benjamingeer benjamingeer self-assigned this Oct 2, 2019
@benjamingeer benjamingeer mentioned this pull request Oct 2, 2019
@subotic
Copy link
Collaborator

subotic commented Oct 3, 2019

I've moved the upgrade code to its own subproject.

@subotic
Copy link
Collaborator

subotic commented Oct 4, 2019

lol, did the same commit at almost the same time :-)

@benjamingeer
Copy link
Author

Great minds think alike. :)

@benjamingeer
Copy link
Author

And thanks for making the subproject!

Before I update the documentation, how do you want to run the Main program? Is it going to be in Docker? If so, should we document how to run it in SBT as well as in Docker?

@lrosenth
Copy link
Contributor

lrosenth commented Oct 4, 2019 via email

@subotic
Copy link
Collaborator

subotic commented Oct 4, 2019

And thanks for making the subproject!

You're welcome :-)

Before I update the documentation, how do you want to run the Main program? Is it going to be in Docker?

Yes, it is going to run in its own Docker image. I would like to run it as the python verson before, i.e., with parameters for GraphDB hostname, repository name and work folder.

If so, should we document how to run it in SBT as well as in Docker?

Documenting Docker is difficult as it depends on the way how it is orchestrated. The way we run it in Docker is just one way of doing it. It works for us at the moment, but will change over time. I think it is enough to document it on how to run it from SBT.

@benjamingeer
Copy link
Author

benjamingeer commented Oct 4, 2019

I would like to run it as the python verson before, i.e., with parameters for GraphDB hostname, repository name and work folder.

I was thinking of giving the sysadmin a bit more control over the process, by having the Scala upgrade program just read an input file and write an output file. In upgrade/graphdb-se there are scripts for dumping a repository to a TriG file, emptying a repository, and uploading a TriG file to a repository, and these scripts take the usual parameters.

This way, if the TriG file was very large, you could use GraphDB's LoadRDF instead of the upload script.

And if Knora rejects your TriG file and you need to fix some things manually in it, you could just do that and upload it again.

Also, the same Scala upgrade program would work with TriG files from other triplestores.

How does that sound?

@benjamingeer benjamingeer requested a review from subotic October 4, 2019 10:43
@subotic
Copy link
Collaborator

subotic commented Oct 4, 2019

I would prefer a full-auto mode, i.e., run upgrade script and wait until it finishes.

Being able to do the upgrade in manual steps is good for debugging if the full-auto mode does not work.

So if possible, then please add both options. Manually working with trig files is error-prone (I have this "pleasure" on each deployment). Having to deal with them additionally for upgrades, is not so ideal.

@benjamingeer
Copy link
Author

please add both options.

OK, will do.

Copy link
Collaborator

@subotic subotic left a comment

Choose a reason for hiding this comment

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

Could you please add the option to optionally provide the path to the directory where stuff is getting downloaded to?

In the old python script there was an option for this: "-t", "--tempdir", help="temporary directory"

Also, it seems that for auto-upgrade.sh the -u and -p options are mandatory, even if they are not needed locally.

@benjamingeer
Copy link
Author

Will do.

@benjamingeer
Copy link
Author

I think I need to get you a wizard costume for Halloween.

@subotic
Copy link
Collaborator

subotic commented Oct 17, 2019

I think I need to get you a wizard costume for Halloween.

lol :-)

@subotic
Copy link
Collaborator

subotic commented Oct 17, 2019

I finally got it working. There is a Travis test covering the sbt upgrade use case. The Docker use case is still missing, but I'm going to add it.

@benjamingeer
Copy link
Author

Also, it seems that for auto-upgrade.sh the -u and -p options are mandatory, even if they are not needed locally.

This is also true with the old upgrade framework. If you don't specify a password, you get a password prompt:

parser.add_argument("-u", "--username", help="GraphDB username", type=str, required=True)
parser.add_argument("-p", "--password", help="GraphDB password (if not provided, will prompt for password)",
                    type=str)

I could add an option --no-auth.

@subotic
Copy link
Collaborator

subotic commented Oct 18, 2019

I could add an option --no-auth.

no, it‘s fine.

Copy link
Collaborator

@subotic subotic left a comment

Choose a reason for hiding this comment

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

I think that this PR is good now. Thanks for making the upgrade faster!

@benjamingeer benjamingeer removed the request for review from loicjaouen October 19, 2019 16:03
@benjamingeer
Copy link
Author

Thanks for the review and the SBT magic!

@benjamingeer benjamingeer merged commit f7d52eb into develop Oct 21, 2019
@benjamingeer benjamingeer deleted the wip/1451-repository-update branch October 21, 2019 07:03
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.

Improve performance of repository upgrade
4 participants