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

Dont index groups (fixes distributed develop) #2572

Merged
merged 17 commits into from May 11, 2021

Conversation

KatrinIhler
Copy link
Member

I recently (?) broke distributed develop by making the userdirectory module depend directly on index modules that only exist on the admin node (while userdirectory is part of the opencast core).

Instead of ripping apart the userdirectory module to make this work, I decided that indexing groups doesn't make much sense anyway, since most institutions don't have that many groups, and we don't index the users right now, which there are probably more of.

So with this PR, we now get the groups directly from the database, and everything regarding group indexing in Elasticsearch is removed. Mostly everything works the same as before, with some minor exceptions:

  • I deprecated sorting by roles and members of groups for the external API (the admin UI does not use this), since it doesn't make much sense and would have made the database query much more complicated. For versions < 1.6.0, the sorting by these fields happens manually afterwards. For this, I introduced a new API version 1.6.0, which might conflict with Retract publications before deleting events #2554.
  • I changed the behavior of the full text search (only available via admin UI) so that if you enter multiple words, all of those words have to match a field in a group, instead of any having to match. This is in violation of the other tables in the admin UI, but I think it makes much more sense and we should rather fix the others some time than re-implement their behavior.
  • I no longer test user permissions, since that was broken before anyway - for non-admins, permissions were tested, but never added, so non-admins could not see groups (even ones they had created themselves) even if they have the correct GROUPS_VIEW role. Now this is only protected by the security config, which should be fine.

I also fixed the group creation in the frontend since it was just one line, now users get correctly added to new groups again.

This should fix #2410, but I didn't test this, maybe somebody else can?

@KatrinIhler KatrinIhler added bug enhancement api change This pull requests changes the Opencast API needs attention We might want to review this fast ELAN Pull requests originating from ELAN e.V. SWITCH java Pull requests that update Java code labels May 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2021

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Copy link
Member

@lkiesow lkiesow left a comment

Choose a reason for hiding this comment

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

I've verified that this fixes #2410 and correctly lists/filters the existing groups in the admin interface.

@gregorydlogan gregorydlogan self-assigned this May 7, 2021
@gregorydlogan
Copy link
Member

@KatrinIhler Does this require a reindex? If so that should be added to the docs. Also, something in the release notes.

Copy link
Member

@gregorydlogan gregorydlogan left a comment

Choose a reason for hiding this comment

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

I'm amused that you removed a ton of code, added very little, and managed to preserve the same basic behaviour. Makes me wonder how useful indexing groups ever was... :)

I haven't tested this thoroughly, but it looks right and my basic tests pass. Will try and get to this again next week, if someone else wants to merge this prior to that feel free.

@KatrinIhler
Copy link
Member Author

@KatrinIhler Does this require a reindex? If so that should be added to the docs. Also, something in the release notes.

I thought about this and I don't think it does? With this change, the groups will be directly picked from the database and new or updated groups won't be indexed, but it shouldn't matter whether you still have some old groups in the index, since they will never be queried, right? Of course a rebuild would be cleaner, but I don't think it's necessary, unless I missed something.

@KatrinIhler
Copy link
Member Author

I'm amused that you removed a ton of code, added very little, and managed to preserve the same basic behaviour. Makes me wonder how useful indexing groups ever was... :)

I think the answer you're looking for is: Not very. :D

@gregorydlogan
Copy link
Member

@KatrinIhler Does this require a reindex? If so that should be added to the docs. Also, something in the release notes.

I thought about this and I don't think it does? With this change, the groups will be directly picked from the database and new or updated groups won't be indexed, but it shouldn't matter whether you still have some old groups in the index, since they will never be queried, right? Of course a rebuild would be cleaner, but I don't think it's necessary, unless I missed something.

I think you're probably right. Ok, let's leave this then.

<provide interface="org.opencastproject.index.service.message.GroupMessageReceiverImpl"/>
</service>

<reference name="message-broker-receiver"
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only module not on an admin node requiring the message broker modules? If so, we should remove them from the assemblies as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Possible, but I don't have time to find that out right now. Can we leave this for later? :D

Copy link
Member

Choose a reason for hiding this comment

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

The admin-ui-index, scheduler-impl, external-api-index, conductor, live-schedule-impl, event-comment, authorization-manager, and themes modules make OSGi references to org.opencastproject.message.broker.api.MessageReceiver. A quick move of the message-broker modules into the admin* and allinone profiles yields booting builds. I haven't had time to do anything other than make sure they start though...

Edit: filed at #2599

@gregorydlogan gregorydlogan merged commit 1c82b71 into opencast:develop May 11, 2021
gregorydlogan added a commit to gregorydlogan/opencast that referenced this pull request May 11, 2021
…ded on the admin node. This *appears* to work for me in terms of booting the nodes, but I have not had time to test more in depth than that.
lkiesow pushed a commit that referenced this pull request May 18, 2021
…he admin node. This *appears* to work for me in terms of booting the nodes, but I have not had time to test more in depth than that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change This pull requests changes the Opencast API bug ELAN Pull requests originating from ELAN e.V. enhancement java Pull requests that update Java code needs attention We might want to review this fast
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Several Opencast 10 Distributions Broken
3 participants