[#11843] Create Logic and Db layer for FeedbackSessionLogs #12914
Conversation
src/main/java/teammates/ui/webapi/UpdateFeedbackSessionLogsAction.java
Outdated
Show resolved
Hide resolved
| Student student = sqlLogic.getStudentForEmail(courseId, email); | ||
| FeedbackSession feedbackSession = sqlLogic.getFeedbackSession(fbSessionName, courseId); |
There was a problem hiding this comment.
I am somewhat worried about the performance of this.
We'll be making 2 queries per log entry, and it's possible that there will be many log entries even with the spam filter.
Apart from the time taken, it may incur alot of reads, making this feature quite expensive.
I think it's possible to optimize this, seeing that there's a higher probability that in an hour, the logs belong to the same feedbacksession / student
There was a problem hiding this comment.
Since it's an optimization problem, it's fine to leave a comment and improve on it later
There was a problem hiding this comment.
Good catch! Since there is a high probability that the logs belong to the same student/session, I have updated the action to store queried students and feedback sessions in a map.
There was a problem hiding this comment.
Since we have changed the logs to store the ids, I changed it to use getReference().
This should make it such that we incur 0 db reads for this part.
Edit:
Since the ids are not validated anywhere we probably need a way to catch the error thrown so other logs will still get created. Can't quite figure out how yet since the error cant seem to be caught by wrapping fslDb.createFeedbackSessionLog() in a try catch
| private final String studentEmail; | ||
| private final String feedbackSessionName; | ||
| private final String feedbackSessionLogType; | ||
| private final long timestamp; | ||
|
|
||
| public FeedbackSessionLogEntry(String studentEmail, String feedbackSessionName, | ||
| String feedbackSessionLogType, long timestamp) { | ||
| public FeedbackSessionLogEntry(String courseId, String studentEmail, |
There was a problem hiding this comment.
@cedricongjh Should GCP log the student's email? I think you mentioned that email is possible to change. If email changes, may break the updating action in future
There was a problem hiding this comment.
ah yes, good point, we should also change the log's format, to log studentId and feedbackSessionId as well, this way, when creating the Entity in SQL, we would only need to query by Id.
For GetFeedbackSessionLogsAction, we should be using studentId and feedbackSessionId too (instead of email and feedbackSessionName)
This will require FE changes (to change the logs format, as well as API request params), so let's change the BE here first and do the FE in a separate PR
src/main/java/teammates/storage/sqlapi/FeedbackSessionLogsDb.java
Outdated
Show resolved
Hide resolved
| Student student = sqlLogic.getStudentForEmail(courseId, studentEmail); | ||
| FeedbackSession feedbackSession = sqlLogic.getFeedbackSession(fsName, courseId); |
There was a problem hiding this comment.
Inorder to log the student and session ids it would incur additional reads on the creation of every log in GCP.
One idea around this is we can update other APIs to pass student/session ids to the frontend so this API can get the ids from the params?
There was a problem hiding this comment.
yes, let's do that! do note that non-migrated courses will not have these attributes, so they will need to set these to null. The frontend will then need to handle this when logging
dishenggg
added
s.ToReview
src/main/java/teammates/common/datatransfer/FeedbackSessionLogEntry.java
Show resolved
Hide resolved
Part of #11843
Outline of Solution
Edit: