nus-oss-test / testrepo4

TEAMMATES system is online at
http://teammatesv4.appspot.com
0 stars 0 forks source link

Verify: not possible for new question to clash with an existing question #1669

Closed damithc closed 10 years ago

damithc commented 10 years ago

From dam...@gmail.com on February 08, 2014 18:47:43

I remember there was an issue about getting an 'already exists' exception when trying to create a new question. Is that issue resolved?

Original issue: http://code.google.com/p/teammatespes/issues/detail?id=1601

damithc commented 10 years ago

From dam...@gmail.com on February 14, 2014 18:09:45

Doesn't seem to be fixed.

Labels: -Priority-Medium Priority-High

damithc commented 10 years ago

From arnold.k...@gmail.com on March 02, 2014 21:32:22

Status: Started

damithc commented 10 years ago

From arnold.k...@gmail.com on March 02, 2014 21:52:13

Ah, missed it the last time but this is possible when all these three fields are identical to an existing question:

  1. feedbackSessionName,
  2. courseId,
  3. questionNumber

which might be triggered if the instructor try to insert a new question in between old questions (e.g. create a new question 1). By right all the old questions should have been shifted to allow the insertion of the new question but maybe because of eventual consistency the error is triggered.

Maybe if the error is encountered we can wait a bit and try again?

Cc: dam...@gmail.com

damithc commented 10 years ago

From dam...@gmail.com on March 02, 2014 22:12:57

It could be an eventual consistency issue. Let's pass this to Shawn to handle. Shawn, see if we can prevent this situation being escalated to the user, and if it does, the user is shown an appropriate message.

Owner: leeshaw...@gmail.com
Cc: -dam...@gmail.com arnold.k...@gmail.com
Labels: FaultTolarence

damithc commented 10 years ago

From arnold.k...@gmail.com on March 03, 2014 21:42:11

updated wrong issue due to typo

Status: Accepted

damithc commented 10 years ago

From leeshaw...@gmail.com on March 06, 2014 21:08:51

Status: Started

damithc commented 10 years ago

From leeshaw...@gmail.com on March 06, 2014 22:08:49

Instead of creating a custom createEntity function which ignores the EntityAlreadyExists exception for this as discussed previously, would it be a better idea to do a swapping of questions?

My idea is since we are already doing updating to the question numbers, how about we do something like this. Let's say we have question 1,2,3,4 and we are inserting a new question in between qns 2 & 3. We do a copy of the new question into the previous qns 3, then copy the previous qns 3 into qns 4, and finally re-insert qns 4 as qns 5 into the DB. This way, we still do the same overall number of updates and 1 write, but we avoid having to do a bypass of the exception as in case it causes a bug in future.

damithc commented 10 years ago

From leeshaw...@gmail.com on March 07, 2014 07:04:21

Made a minor change to the way a feedback question is added. The question entity is now created first, then inserted into the correct position instead of shifting all the questions before creating the question entity. Changing this order will allow the the entity to be created without triggering the entity already exist exception if the shifting of question numbers is affected by eventual consistency. Also added a little more test coverage for existing test.

Code is ready for review at: https://codereview.appspot.com/72300045

Status: ReadyForReview

damithc commented 10 years ago

From dam...@gmail.com on March 07, 2014 19:35:50

Here's the relevant stacktrace from a previous issue. i don't think the current patch solves this issue. I think we need a new method at *Db level for persisting a bunch of questions in one go. Changing existing questions and adding new question in multiple writes is always vulnerable to this issue. This is because the current createEntity method checks for existing entities before creating a new one. There is no guarantee that the question number modification is persisted before we try to write the new question.

teammates.ui.controller.ControllerServlet doPost: TEAMMATESLOG|||page|||System Error Report|||true|||Unknown|||Unknown|||Unknown|||Unknown|||TEAMMATES (4.81): New System Exception: Creating a duplicate question should not be possible as GAE generates a new questionId every time
Show/Hide Details >>


Creating a duplicate question should not be possible as GAE generates a new questionId every time


Request Path /page

Request Parameters {noofchoicecreated::3//, showresponsesto::RECEIVER,INSTRUCTORS, showrecipientto::RECEIVER,INSTRUCTORS, numofrecipients::1, questiontext:: attuned one’s own activities to the activities of others, fsname::blabla, numofrecipientstype::max, mcqOption-1::0, msqOption-0::, mcqOption-2::-1, recipienttype::STUDENTS, showgiverto::INSTRUCTORS, mcqOption-0::1, msqOption-1::, receiverLeaderCheckbox::RECEIVER, questiontype::MCQ, generatedOptions::NONE, questionnum::2//2, givertype::STUDENTS, user::abc, courseid::Comm.Campaigns.2013}

Stack Trace


java.lang.AssertionError: Creating a duplicate question should not be possible as GAE generates a new questionId every time
    at teammates.common.util.Assumption.fail(Assumption.java:56)
    at teammates.ui.controller.InstructorFeedbackQuestionAddAction.execute(InstructorFeedbackQuestionAddAction.java:45)
    at teammates.ui.controller.Action.executeAndPostProcess(Action.java:125)
    at teammates.ui.controller.ControllerServlet.doPost(ControllerServlet.java:48)
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:637)
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
    at org.mortbay.jetty.servlet.ServletHolder.handle(ServletHolder.java:511)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1166)
    at teammates.ui.controller.LoginFilter.doFilter(LoginFilter.java:46)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at teammates.storage.datastore.DatastoreFilter.doFilter(DatastoreFilter.java:28)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at com.google.appengine.tools.appstats.AppstatsFilter.doFilter(AppstatsFilter.java:141)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at com.google.apphosting.utils.servlet.ParseBlobUploadFilter.doFilter(ParseBlobUploadFilter.java:125)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at com.google.apphosting.runtime.jetty.SaveSessionFilter.doFilter(SaveSessionFilter.java:35)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at com.google.apphosting.utils.servlet.JdbcMySqlConnectionCleanupFilter.doFilter(JdbcMySqlConnectionCleanupFilter.java:60)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at com.google.apphosting.utils.servlet.TransactionCleanupFilter.doFilter(TransactionCleanupFilter.java:43)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at org.mortbay.jetty.servlet.ServletHandler.handle(ServletHandler.java:388)
    at org.mortbay.jetty.security.SecurityHandler.handle(SecurityHandler.java:216)
    at org.mortbay.jetty.servlet.SessionHandler.handle(SessionHandler.java:182)
    at org.mortbay.jetty.handler.ContextHandler.handle(ContextHandler.java:765)
    at org.mortbay.jetty.webapp.WebAppContext.handle(WebAppContext.java:418)
    at com.google.apphosting.runtime.jetty.AppVersionHandlerMap.handle(AppVersionHandlerMap.java:266)
    at org.mortbay.jetty.handler.HandlerWrapper.handle(HandlerWrapper.java:152)
    at org.mortbay.jetty.Server.handle(Server.java:326)
    at org.mortbay.jetty.HttpConnection.handleRequest(HttpConnection.java:542)
    at org.mortbay.jetty.HttpConnection$RequestHandler.headerComplete(HttpConnection.java:923)
    at com.google.apphosting.runtime.jetty.RpcRequestParser.parseAvailable(RpcRequestParser.java:76)
    at org.mortbay.jetty.HttpConnection.handle(HttpConnection.java:404)
    at com.google.apphosting.runtime.jetty.JettyServletEngineAdapter.serviceRequest(JettyServletEngineAdapter.java:146)
    at com.google.apphosting.runtime.JavaRuntime$RequestRunnable.run(JavaRuntime.java:446)
    at com.google.tracing.TraceContext$TraceContextRunnable.runInContext(TraceContext.java:437)
    at com.google.tracing.TraceContext$TraceContextRunnable$1.run(TraceContext.java:444)
    at com.google.tracing.CurrentContext.runInContext(CurrentContext.java:188)
    at com.google.tracing.TraceContext$AbstractTraceContextCallback.runInInheritedContextNoUnref(TraceContext.java:308)
    at com.google.tracing.TraceContext$AbstractTraceContextCallback.runInInheritedContext(TraceContext.java:300)
    at com.google.tracing.TraceContext$TraceContextRunnable.run(TraceContext.java:441)
    at com.google.apphosting.runtime.ThreadGroupPool$PoolEntry.run(ThreadGroupPool.java:251)
    at java.lang.Thread.run(Thread.java:724)

|||/page/instructorFeedbackQuestionAdd

Status: ChangesRequested

damithc commented 10 years ago

From leeshaw...@gmail.com on March 07, 2014 21:03:05

Added a method to the entitiesDb base class that allows the creation of an entity without checking for existing entities to facilitate creating a new entity for methods with many updates before a create.

Code ready for review at: https://codereview.appspot.com/72300045

Status: ReadyForReview

damithc commented 10 years ago

From dam...@gmail.com on March 07, 2014 21:58:34

Status: ReadyToMerge

damithc commented 10 years ago

From leeshaw...@gmail.com on March 07, 2014 23:18:11

This issue was updated by revision 8408900c4a49 .

damithc commented 10 years ago

From leeshaw...@gmail.com on March 07, 2014 23:18:20

Status: Delivered

damithc commented 10 years ago

From dam...@gmail.com on March 14, 2014 23:34:42

Status: Deployed
Labels: Milestone-V4.91