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

[#11585] Fix unnecessary data read in expected submission count #11659

Merged
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
97a5284
update: add get number of students for course id
moziliar Mar 20, 2022
2646bcb
update: add method to only retrieve instructor emails in a course
moziliar Mar 20, 2022
fd9ac58
update: add methods to get aggregate count of questions by giver types
moziliar Mar 20, 2022
08383c7
fix: remove projection query that requires expensive index
moziliar Mar 20, 2022
15533f6
update: change all Long to Int
moziliar Mar 21, 2022
474fecd
nit: change return type to unboxed int
moziliar Mar 21, 2022
ffd86ba
nit: change assert false to assert not equal for equality check
moziliar Mar 21, 2022
3129653
nit: change name of var and javadoc to include email
moziliar Mar 21, 2022
dfc06d5
nit: fix typo
moziliar Mar 21, 2022
fb7e683
Merge branch 'master' into 11585-fix-unnecessary-data-read
moziliar Mar 21, 2022
4041ef8
nit: fix typo
moziliar Mar 21, 2022
f3c2edf
nit: change type to int wherever appropriate
moziliar Mar 21, 2022
0c165b2
nit: fix typo and revert unrelated typo fix
moziliar Mar 22, 2022
35a490a
update: use hasQuestionForX in place of counting giver type
moziliar Mar 22, 2022
37dcec8
update: reduce more redundant data read from checking presence of ins…
moziliar Mar 26, 2022
36d95bf
update: reduce data read
moziliar Mar 26, 2022
6692fe9
Merge branch 'master' into 11585-fix-unnecessary-data-read
moziliar Mar 27, 2022
c4888b8
refactor: refactor getExpectedTotalSubmission to counting instructor …
moziliar Mar 27, 2022
9e1ff59
refactor: reorder code to make data read only when necessary
moziliar Mar 27, 2022
cbcd27f
refactor: remove duplicate condition
moziliar Mar 27, 2022
5473d99
Merge branch 'master' into 11585-fix-unnecessary-data-read
wkurniawan07 Mar 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/main/java/teammates/logic/core/FeedbackQuestionsLogic.java
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,31 @@ public List<FeedbackQuestionAttributes> getFeedbackQuestionsForSession(
return questions;
}

/**
* Gets the number of questions for student to answer from a map of giver type to question counts.
*/
public int getFeedbackQuestionForStudentCountFrom(Map<FeedbackParticipantType, Integer> giverTypeCounts) {
return giverTypeCounts.getOrDefault(FeedbackParticipantType.STUDENTS, 0)
+ giverTypeCounts.getOrDefault(FeedbackParticipantType.TEAMS, 0);
}

/**
* Gets the number of questions for instructor to answer from a map of giver type to question counts.
*/
public int getFeedbackQuestionForInstructorCountFrom(
Map<FeedbackParticipantType, Integer> giverTypeCounts, boolean isCreator) {
return giverTypeCounts.getOrDefault(FeedbackParticipantType.INSTRUCTORS, 0)
+ (isCreator ? giverTypeCounts.getOrDefault(FeedbackParticipantType.SELF, 0) : 0);
}

/**
* Gets a {@link Map} of FeedbackQuestion giver types to count in the given session.
*/
public Map<FeedbackParticipantType, Integer> getFeedbackQuestionGiverCountForSession(
String feedbackSessionName, String courseId) {
return fqDb.getFeedbackQuestionGiverTypeCountForSession(feedbackSessionName, courseId);
}

// TODO can be removed once we are sure that question numbers will be consistent
private boolean areQuestionNumbersConsistent(List<FeedbackQuestionAttributes> questions) {
Set<Integer> questionNumbersInSession = new HashSet<>();
Expand Down
22 changes: 9 additions & 13 deletions src/main/java/teammates/logic/core/FeedbackSessionsLogic.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import teammates.common.datatransfer.AttributesDeletionQuery;
import teammates.common.datatransfer.FeedbackParticipantType;
import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes;
import teammates.common.datatransfer.attributes.FeedbackSessionAttributes;
import teammates.common.datatransfer.attributes.InstructorAttributes;
import teammates.common.datatransfer.attributes.StudentAttributes;
import teammates.common.exception.EntityAlreadyExistsException;
import teammates.common.exception.EntityDoesNotExistException;
import teammates.common.exception.InvalidParametersException;
Expand Down Expand Up @@ -492,22 +492,18 @@ public void restoreFeedbackSessionFromRecycleBin(String feedbackSessionName, Str
* Gets the expected number of submissions for a feedback session.
*/
public int getExpectedTotalSubmission(FeedbackSessionAttributes fsa) {
List<StudentAttributes> students = studentsLogic.getStudentsForCourse(fsa.getCourseId());
List<InstructorAttributes> instructors = instructorsLogic.getInstructorsForCourse(fsa.getCourseId());
List<FeedbackQuestionAttributes> questions =
fqLogic.getFeedbackQuestionsForSession(fsa.getFeedbackSessionName(), fsa.getCourseId());
List<FeedbackQuestionAttributes> studentQns = fqLogic.getFeedbackQuestionsForStudents(questions);
int numOfStudents = studentsLogic.getNumberOfStudentsForCourse(fsa.getCourseId());
List<String> instructorEmails = instructorsLogic.getInstructorEmailsForCourse(fsa.getCourseId());
moziliar marked this conversation as resolved.
Show resolved Hide resolved
Map<FeedbackParticipantType, Integer> giverTypeCounts = fqLogic.getFeedbackQuestionGiverCountForSession(
fsa.getFeedbackSessionName(), fsa.getCourseId());

int expectedTotal = 0;

if (!studentQns.isEmpty()) {
expectedTotal += students.size();
if (fqLogic.getFeedbackQuestionForStudentCountFrom(giverTypeCounts) > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I just realized, you should be able to make use of hasFeedbackQuestionsForStudents here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right... I just made those methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just removed the giverTypeCount method and replace with overloaded hasQnForX

Copy link
Member

Choose a reason for hiding this comment

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

I don't think even the overloaded method is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other hasFeedbackQuestionsForInstructors has isCreatorOfSession which in turn fetches the FeedbackSession entity per call. I don't feel like this is justified when we literally have the fsa available in the caller.

Copy link
Member

Choose a reason for hiding this comment

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

Then, that other method is what you should modify

Copy link
Member

Choose a reason for hiding this comment

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

Even then:

  • Note that any attribute object can be easily created through means other than fetching from DB. As much as it may seem "wasted", fetching the item every time is far less fault-tolerant.
  • In addition, you should notice that calling hasFeedbackQuestionsForInstructors for each instructor is in itself already wasted. Only one kind of instructor can possibly produce different outcome and that is the creator instructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right about this. I should just eliminate down to only necessary information needed from db.

Note that any attribute object can be easily created through means other than fetching from DB. As much as it may seem "wasted", fetching the item every time is far less fault-tolerant.

My take on this is that we want a snapshot of a session from one point in time. Fetching the session entity multiple times along the way might provide some inconsistency instead IMO.
Any ways we can guard against session entity not fetched from DB?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I forgot to reply this.

Probably I shouldn't mention the first reason. It is definitely a concern, but another factor being in play is what the method is supposed to do. For instance, do you want a method that "does something with this given Attribute object", or "does something with this course ID and session name"? They can very well do different things even though in a glance they sound the same.
But determining which one is the right one to use is not a black-and-white matter. It is highly situational and depends on the business logic at the point of the method call. Which is why, "[session] entity not fetched from DB" is not even necessarily a problem.

But the second reason still stands as to why there is still room for even more optimization here.

expectedTotal += numOfStudents;
}

for (InstructorAttributes instructor : instructors) {
List<FeedbackQuestionAttributes> instructorQns =
fqLogic.getFeedbackQuestionsForInstructors(questions, fsa.isCreator(instructor.getEmail()));
if (!instructorQns.isEmpty()) {
for (String instructorEmail : instructorEmails) {
if (fqLogic.getFeedbackQuestionForInstructorCountFrom(giverTypeCounts, fsa.isCreator(instructorEmail)) > 0) {
moziliar marked this conversation as resolved.
Show resolved Hide resolved
expectedTotal += 1;
}
}
Expand Down
11 changes: 11 additions & 0 deletions src/main/java/teammates/logic/core/InstructorsLogic.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package teammates.logic.core;

import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;

import teammates.common.datatransfer.AttributesDeletionQuery;
Expand Down Expand Up @@ -122,6 +123,16 @@ public InstructorAttributes getInstructorForRegistrationKey(String registrationK
return instructorsDb.getInstructorForRegistrationKey(registrationKey);
}

/**
* Gets emails of all instructors of a course.
*/
public List<String> getInstructorEmailsForCourse(String courseId) {
List<String> instructorEmails = instructorsDb.getInstructorEmailsForCourse(courseId);
instructorEmails.sort(Comparator.naturalOrder());

return instructorEmails;
}

/**
* Gets all instructors of a course.
*/
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/teammates/logic/core/StudentsLogic.java
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ public List<StudentAttributes> getStudentsForGoogleId(String googleId) {
return studentsDb.getStudentsForGoogleId(googleId);
}

/**
* Gets the total number of students of a course.
*/
public int getNumberOfStudentsForCourse(String courseId) {
return studentsDb.getNumberOfStudentsForCourse(courseId);
}

/**
* Gets all students of a course.
*/
Expand Down
18 changes: 18 additions & 0 deletions src/main/java/teammates/storage/api/FeedbackQuestionsDb.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import static com.googlecode.objectify.ObjectifyService.ofy;

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import com.googlecode.objectify.cmd.LoadType;
import com.googlecode.objectify.cmd.Query;
Expand Down Expand Up @@ -52,6 +54,22 @@ public FeedbackQuestionAttributes getFeedbackQuestion(
return makeAttributesOrNull(getFeedbackQuestionEntity(feedbackSessionName, courseId, questionNumber));
}

/**
* Gets feedback question count grouped by giver types of a session.
*/
public Map<FeedbackParticipantType, Integer> getFeedbackQuestionGiverTypeCountForSession(
String feedbackSessionName, String courseId) {
assert feedbackSessionName != null;
assert courseId != null;

return load()
.filter("feedbackSessionName =", feedbackSessionName)
.filter("courseId =", courseId)
.list()
.stream()
.collect(Collectors.groupingBy(FeedbackQuestion::getGiverType, Collectors.summingInt(x -> 1)));
}

/**
* Gets all feedback questions of a session.
*/
Expand Down
15 changes: 15 additions & 0 deletions src/main/java/teammates/storage/api/InstructorsDb.java
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,21 @@ public List<InstructorAttributes> getInstructorsForGoogleId(String googleId, boo
return makeAttributes(getInstructorEntitiesForGoogleId(googleId, omitArchived));
}

/**
* Gets the emails of all instructors of a course.
*/
public List<String> getInstructorEmailsForCourse(String courseId) {
assert courseId != null;

return load()
.filter("courseId =", courseId)
.project("email")
.list()
.stream()
.map(Instructor::getEmail)
.collect(Collectors.toList());
}

/**
* Gets all instructors of a course.
*/
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/teammates/storage/api/StudentsDb.java
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,15 @@ public List<StudentAttributes> getStudentsForGoogleId(String googleId) {
return makeAttributes(getCourseStudentEntitiesForGoogleId(googleId));
}

/**
* Gets the total number of students of a course.
*/
public int getNumberOfStudentsForCourse(String courseId) {
assert courseId != null;

return getCourseStudentsForCourseQuery(courseId).count();
}

/**
* Gets all students of a course.
*/
Expand Down
36 changes: 36 additions & 0 deletions src/test/java/teammates/logic/core/FeedbackQuestionsLogicTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public void testDeleteFeedbackQuestions_byCourseIdAndSessionName_shouldDeleteQue

@Test
public void allTests() throws Exception {
testGetFeedbackQuestionGiverCount();
testHasFeedbackQuestionsForInstructor();
testGetFeedbackQuestionsForInstructor();
testHasFeedbackQuestionsForStudents();
Expand Down Expand Up @@ -907,6 +908,41 @@ public void testBuildCompleteGiverRecipientMap_teamQuestion_shouldBuildMapCorrec
assertTrue(completeGiverRecipientMap.get("Team 1.2").contains("Team 1.1</td></div>'\""));
}

private void testGetFeedbackQuestionGiverCount() {
______TS("Valid session valid course should give correct counts");
Map<FeedbackParticipantType, Integer> giverTypeCounts = fqLogic.getFeedbackQuestionGiverCountForSession(
"First feedback session", "idOfTypicalCourse1");
int studentTypeCount = giverTypeCounts.get(FeedbackParticipantType.STUDENTS);
int instructorTypeCount = giverTypeCounts.get(FeedbackParticipantType.INSTRUCTORS);
int selfTypeCount = giverTypeCounts.get(FeedbackParticipantType.SELF);
assertEquals(2, studentTypeCount);
assertEquals(1, instructorTypeCount);
assertEquals(2, selfTypeCount);

assertEquals(studentTypeCount, fqLogic.getFeedbackQuestionForStudentCountFrom(giverTypeCounts));

assertEquals(instructorTypeCount,
fqLogic.getFeedbackQuestionForInstructorCountFrom(giverTypeCounts, false));
assertEquals(instructorTypeCount + selfTypeCount,
fqLogic.getFeedbackQuestionForInstructorCountFrom(giverTypeCounts, true));

______TS("Invalid session valid course should give no count");
giverTypeCounts = fqLogic.getFeedbackQuestionGiverCountForSession("invalid session", "idOfTypicalCourse1");
assertEquals(0, giverTypeCounts.size());

assertEquals(0, fqLogic.getFeedbackQuestionForStudentCountFrom(giverTypeCounts));
assertEquals(0, fqLogic.getFeedbackQuestionForInstructorCountFrom(giverTypeCounts, false));
assertEquals(0, fqLogic.getFeedbackQuestionForInstructorCountFrom(giverTypeCounts, true));

______TS("Invalid session invalid course should give no count");
giverTypeCounts = fqLogic.getFeedbackQuestionGiverCountForSession("invalid session", "invalid course");
assertEquals(0, giverTypeCounts.size());

assertEquals(0, fqLogic.getFeedbackQuestionForStudentCountFrom(giverTypeCounts));
assertEquals(0, fqLogic.getFeedbackQuestionForInstructorCountFrom(giverTypeCounts, false));
assertEquals(0, fqLogic.getFeedbackQuestionForInstructorCountFrom(giverTypeCounts, true));
}

private void testHasFeedbackQuestionsForInstructor() {
______TS("Valid session valid instructor should have questions");
assertTrue(fqLogic.hasFeedbackQuestionsForInstructors(
Expand Down
26 changes: 26 additions & 0 deletions src/test/java/teammates/logic/core/StudentsLogicTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public void testAll() throws Exception {
testGetStudentForRegistrationKey();
testGetStudentsForGoogleId();
testGetStudentForCourseIdAndGoogleId();
testGetNumberOfStudentsForCourse();
testGetStudentsForCourse();
testIsStudentInAnyCourse();
testIsStudentInTeam();
Expand Down Expand Up @@ -411,6 +412,31 @@ private void testGetStudentForCourseIdAndGoogleId() {
() -> studentsLogic.getStudentForCourseIdAndGoogleId("valid.course", null));
}

private void testGetNumberOfStudentsForCourse() {

______TS("course with multiple students");

CourseAttributes course1OfInstructor1 = dataBundle.courses.get("typicalCourse1");
int numOfStudents = studentsLogic.getNumberOfStudentsForCourse(course1OfInstructor1.getId());
assertEquals(5, numOfStudents);

______TS("course with 0 students");

CourseAttributes course2OfInstructor1 = dataBundle.courses.get("courseNoEvals");
numOfStudents = studentsLogic.getNumberOfStudentsForCourse(course2OfInstructor1.getId());
assertEquals(0, numOfStudents);

______TS("null parameter");

assertThrows(AssertionError.class, () -> studentsLogic.getNumberOfStudentsForCourse(null));

______TS("non-existent course");

numOfStudents = studentsLogic.getNumberOfStudentsForCourse("non-existent");
assertEquals(0, numOfStudents);

}

private void testGetStudentsForCourse() {

______TS("course with multiple students");
Expand Down
44 changes: 43 additions & 1 deletion src/test/java/teammates/storage/api/FeedbackQuestionsDbTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import org.testng.annotations.Test;
import org.testng.collections.Lists;
Expand Down Expand Up @@ -286,6 +287,47 @@ public void testGetFeedbackQuestions() throws Exception {
assertNull(actual);
}

@Test
public void testGetFeedbackQuestionGiverTypeCountsForSession() throws Exception {
______TS("standard success case");
FeedbackQuestionAttributes fqa = getNewFeedbackQuestionAttributes();

// remove possibly conflicting entity from the database
deleteFeedbackQuestion(fqa);

int[] numOfQuestions = createNewQuestionsForDifferentRecipientTypes();

Map<FeedbackParticipantType, Integer> giverTypeCounts =
fqDb.getFeedbackQuestionGiverTypeCountForSession(
fqa.getFeedbackSessionName(),
fqa.getCourseId());

assertEquals(Integer.valueOf(numOfQuestions[0]), giverTypeCounts.get(FeedbackParticipantType.INSTRUCTORS));
assertEquals(Integer.valueOf(numOfQuestions[1]), giverTypeCounts.get(FeedbackParticipantType.STUDENTS));
assertEquals(Integer.valueOf(numOfQuestions[2]), giverTypeCounts.get(FeedbackParticipantType.SELF));
assertEquals(Integer.valueOf(numOfQuestions[3]), giverTypeCounts.get(FeedbackParticipantType.TEAMS));

______TS("null params");

assertThrows(AssertionError.class,
() -> fqDb.getFeedbackQuestionGiverTypeCountForSession(null, fqa.getCourseId()));

assertThrows(AssertionError.class,
() -> fqDb.getFeedbackQuestionGiverTypeCountForSession(fqa.getFeedbackSessionName(), null));

______TS("non-existent session");

assertTrue(fqDb.getFeedbackQuestionGiverTypeCountForSession(
"non-existant session", fqa.getCourseId()).isEmpty());
moziliar marked this conversation as resolved.
Show resolved Hide resolved

______TS("no questions in session");

assertTrue(fqDb.getFeedbackQuestionGiverTypeCountForSession(
"Empty session", fqa.getCourseId()).isEmpty());

deleteFeedbackQuestions(numOfQuestions[0] + numOfQuestions[1] + numOfQuestions[2] + numOfQuestions[3]);
}

@Test
public void testGetFeedbackQuestionsForSession() throws Exception {

Expand Down Expand Up @@ -364,7 +406,7 @@ public void testGetFeedbackQuestionsForGiverType() throws Exception {
assertThrows(AssertionError.class,
() -> fqDb.getFeedbackQuestionsForGiverType(fqa.getFeedbackSessionName(), fqa.getCourseId(), null));

______TS("non-existant session");
______TS("non-existent session");
moziliar marked this conversation as resolved.
Show resolved Hide resolved

assertTrue(fqDb.getFeedbackQuestionsForGiverType("non-existant session", fqa.getCourseId(),
FeedbackParticipantType.STUDENTS).isEmpty());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ private void testGetFeedbackSessions() {

assertEquals(expected.toString(), actual.toString());

______TS("non-existant session");
______TS("non-existent session");
moziliar marked this conversation as resolved.
Show resolved Hide resolved

assertNull(fsDb.getFeedbackSession("non-course", "Non-existant feedback session"));

Expand Down
25 changes: 25 additions & 0 deletions src/test/java/teammates/storage/api/InstructorsDbTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,31 @@ public void testGetInstructorsForGoogleId() throws Exception {

}

@Test
public void testGetInstructorEmailsForCourse() {

______TS("Success: get instructors of a specific course");

String courseId = "idOfTypicalCourse1";

List<String> emails = instructorsDb.getInstructorEmailsForCourse(courseId);
List<InstructorAttributes> instructors = instructorsDb.getInstructorsForCourse(courseId);
assertEquals(5, emails.size());
assertEquals(5, instructors.size());
for (var instructor : instructors) {
assertTrue(emails.contains(instructor.getEmail()));
}

______TS("Failure: no instructors for a course");

emails = instructorsDb.getInstructorEmailsForCourse("non-exist-course");
assertEquals(0, emails.size());

______TS("Failure: null parameters");

assertThrows(AssertionError.class, () -> instructorsDb.getInstructorEmailsForCourse(null));
}

@Test
public void testGetInstructorsForCourse() {

Expand Down
Loading