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.

Sign up for GitHub

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

Jump to bottom

[#11020] Make search service optional and remove App Engine Search API usage #11067

Merged
merged 8 commits into from
Mar 29, 2021
Merged

[#11020] Make search service optional and remove App Engine Search API usage #11067

merged 8 commits into from
Mar 29, 2021

Conversation

wkurniawan07
Copy link
Member

@wkurniawan07 wkurniawan07 commented Mar 27, 2021 •
edited

First part of #11020

  • Restructure the storage layer such that search-specific libraries are used strictly in storage.search package only.
  • Refactor code such that search service is optional; if there is no search service:
    • All search-related tests will be skipped. At the same time, it will activate some specific test cases to ensure that absence of search service will not cause problems.
    • Storage layer will either skip the operation and log at warning level, or throw SearchNotImplementedException. The latter will be translated to 501 Not Implemented status at API layer.
  • Temporarily remove all specific search implementations, which essentially removes all search functions in the back-end. One can confirm that there is no more usage of com.google.appengine.api.search package in the code base.

@wkurniawan07
Remove searching response comments feature
b2e8bfd
@wkurniawan07
Tighten architecture rule for GAE search API
394dd8a
@wkurniawan07
Use factory to register search manager
7246f4c
@wkurniawan07
Add skeleton search service connection
1316022
@wkurniawan07
Remove all search-related items temporarily
68efc70
@wkurniawan07
Merge branch 'search-api-migration' into runtime-upgrade/search-prep
f4029f9 
# Conflicts:
#	src/e2e/java/teammates/e2e/cases/BaseE2ETestCase.java
#	src/e2e/java/teammates/e2e/cases/InstructorHomePageE2ETest.java
#	src/e2e/java/teammates/e2e/pageobjects/InstructorSearchPage.java
#	src/main/java/teammates/common/util/Const.java
#	src/main/java/teammates/storage/search/SearchDocument.java
#	src/test/java/teammates/test/GaeSimulation.java
#	src/test/java/teammates/test/TestProperties.java
#	src/test/resources/test.template.properties
@wkurniawan07
Merge no-service test cases to the original test file
bc3e76d
@wkurniawan07
Change log level for no-service to warning
89bd1cb
@wkurniawan07 wkurniawan07 added this to In progress in GAE Runtime Upgrade via automation Mar 27, 2021
@wkurniawan07 wkurniawan07 added the s.ToReview The PR is waiting for review(s) label Mar 27, 2021
@wkurniawan07 wkurniawan07 modified the milestone: V8.0.0 Mar 27, 2021
Derek-Hardy
Derek-Hardy reviewed Mar 27, 2021
View reviewed changes
src/main/java/teammates/storage/search/SearchManager.java
private SearchManager() {
// utility class
private boolean isSearchServiceActive() {
return !StringHelper.isEmpty(searchServiceHost);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Contrary to the TestProperties counterpart, return value of this function will always be negated.
How about isSearchServiceInactive()?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Derek-Hardy I'll leave this to later edit, if absolutely necessary

src/main/java/teammates/storage/search/SearchManagerFactory.java
@SuppressWarnings("PMD.NonThreadSafeSingleton") // ok to ignore as method is only invoked at application startup
public static void registerSearchManager(SearchManager searchManager) {
if (instance == null) {
instance = searchManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for null check? Intuitively the registration should overwrite whatever the old instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

The instance is final and should not be changed once set in the entire application lifecycle. Since using final obviously will not work here, this is the best alternative.

@Derek-Hardy Derek-Hardy self-assigned this Mar 27, 2021
jianhandev
jianhandev approved these changes Mar 28, 2021
View reviewed changes
Copy link
Contributor

@jianhandev jianhandev left a comment

Choose a reason for hiding this comment

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

LGTM.

@wkurniawan07 wkurniawan07 merged commit 9afd274 into search-api-migration Mar 29, 2021
GAE Runtime Upgrade automation moved this from In progress to Done in feature branch Mar 29, 2021
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Mar 29, 2021
@wkurniawan07 wkurniawan07 deleted the runtime-upgrade/search-prep branch March 30, 2021 17:32
wkurniawan07 added a commit that referenced this pull request Apr 12, 2021
@wkurniawan07
[#11020] Make search service optional and remove App Engine Search AP…
ad86a6b 
…I usage (#11067)
wkurniawan07 added a commit that referenced this pull request Apr 12, 2021
@wkurniawan07
[#11020] Make search service optional and remove App Engine Search AP…
7a8d2e3 
…I usage (#11067)
@wkurniawan07 wkurniawan07 added this to the V8.0.0 milestone Apr 25, 2021
@wkurniawan07 wkurniawan07 removed this from Done in feature branch in GAE Runtime Upgrade Apr 26, 2021
wkurniawan07 added a commit that referenced this pull request Apr 28, 2021
@wkurniawan07
[#11020] Make search service optional and remove App Engine Search AP…
0f61160 
…I usage (#11067)
wkurniawan07 added a commit that referenced this pull request May 3, 2021
@wkurniawan07
[#11020] Make search service optional and remove App Engine Search AP…
597313f 
…I usage (#11067)
wkurniawan07 added a commit that referenced this pull request Jun 2, 2021
@wkurniawan07
[#11020] Make search service optional and remove App Engine Search AP…
d6d77bb 
…I usage (#11067)
wkurniawan07 added a commit that referenced this pull request Jun 6, 2021
@wkurniawan07
[#11020] Make search service optional and remove App Engine Search AP…
e09a9d4 
…I usage (#11067)
@wkurniawan07 wkurniawan07 added c.Task Other non-user-facing works, e.g. refactoring, adding tests c.DevOps Process-related or build-related improvement and addition and removed c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Jun 6, 2021
wkurniawan07 added a commit that referenced this pull request Jun 6, 2021
@wkurniawan07
[#11020] Make search service optional and remove App Engine Search AP…
b5e0a88 
…I usage (#11067)
daongochieu2810 pushed a commit to daongochieu2810/teammates that referenced this pull request Jul 5, 2021
@wkurniawan07 @daongochieu2810
[TEAMMATES#11020] Make search service optional and remove App Engine …
49e0ea7 
…Search API usage (TEAMMATES#11067)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Derek-Hardy Derek-Hardy Derek-Hardy left review comments

@jianhandev jianhandev jianhandev approved these changes

Assignees

@Derek-Hardy Derek-Hardy

Labels
c.DevOps Process-related or build-related improvement and addition s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V8.0.0-beta.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@wkurniawan07 @Derek-Hardy @jianhandev