> On Feb. 26, 2016, 12:29 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/orm/AmbariJpaLocalTxnInterceptor.java, > > lines 284-287 > > <https://reviews.apache.org/r/43967/diff/5/?file=1272172#file1272172line284> > > > > If we end up in a nested call here, can this ever be the same lock > > instances, and if so will calling lock() repeatedly work? > > Jonathan Hurley wrote: > We can and I think it's OK. Because we're using a ReentrantLock here, we > can call lock() as many times as we want and it will increase the counters in > the lock. Does that answer your question? > > My unit test should also be able to test this by invoking a method with a > nested Transactional and ensuring that there's no deadlock waiting.
Just bringing it up :) > On Feb. 26, 2016, 12:29 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/orm/AmbariJpaLocalTxnInterceptor.java, > > lines 76-78 > > <https://reviews.apache.org/r/43967/diff/5/?file=1272172#file1272172line76> > > > > I'm assuming it's a Set for nested @TransactionactionalLock's? default > > equals()/hashCode() sufficient? > > Jonathan Hurley wrote: > Yes, according to the javadoc for the Annotation base class, it is. > > https://docs.oracle.com/javase/7/docs/api/java/lang/annotation/Annotation.html#equals(java.lang.Object) > > I'm adding tests for this as well to ensure it works the way we think. Ok > On Feb. 26, 2016, 12:29 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/orm/AmbariJpaLocalTxnInterceptor.java, > > lines 308-310 > > <https://reviews.apache.org/r/43967/diff/5/?file=1272172#file1272172line308> > > > > If nested, will these unlock in the same order they were inserted? > > Maybe consider a LinkedHashSet to preserve it and iterate backwards such > > that you unlock the last one locked? > > Jonathan Hurley wrote: > Interesting use case; Right now the annotation only allows for a single > LockArea, but what if you hit foo() with LockArea.ONE and bar() with > LockArea.TWO; yes, they should probably be done in reverse. I'll make this > change and re-submit. Ok :) - Nate ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43967/#review120890 ----------------------------------------------------------- On Feb. 26, 2016, 12:22 p.m., Jonathan Hurley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43967/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2016, 12:22 p.m.) > > > Review request for Ambari, Alejandro Fernandez, Nate Cole, Sumit Mohanty, > Sebastian Toader, and Sid Wagle. > > > Bugs: AMBARI-15173 > https://issues.apache.org/jira/browse/AMBARI-15173 > > > Repository: ambari > > > Description > ------- > > Seen while performing an upgrade, it's possible that the status of a > request/stage does not match that of its tasks. Essentially, the task could > be {{HOLDING}} while the request is still {{IN_PROGRESS}}. > > I believe that AMBARI-15011 is responsible for this issue. AMBARI-15011 > introduced, among other things, a cache to the > {{HostRoleCommandStatusSummaryDTO}} which is a aggregation of the number of > tasks a stage has in each state (PENDING, HOLDING, etc). > > This {{HostRoleCommandStatusSummaryDTO}} is used by {{CalculatedState}} to > calculate a stage's and request's status based on the tasks. > > The problem is that {{ServerActionExecutor}} is moving a tasks's state to > {{HOLDING}} (reflected in the database correctly) but the cache invalidation > happens inside the uncommitted transaction. This causes stale data to be > re-cached. So, when we go to calculate the request and state status, we get > {{IN_PROGRESS}} instead of {{HOLDING}}. > > {code} > { > "href": > "http://172.22.72.13:8080/api/v1/clusters/cl1/requests/61/stages/1?fields=*,tasks/*";, > "Stage": { > "cluster_name": "cl1", > "context": "Stop YARN Queues", > "display_status": "IN_PROGRESS", > "end_time": -1, > "progress_percent": 35, > "request_id": 61, > "skippable": true, > "stage_id": 1, > "start_time": 1456227329191, > "status": "IN_PROGRESS" > }, > "tasks": [ > { > "href": > "http://172.22.72.13:8080/api/v1/clusters/cl1/requests/61/stages/1/tasks/754";, > "Tasks": { > "attempt_cnt": 1, > "cluster_name": "cl1", > "command": "EXECUTE", > "command_detail": "Before continuing, please stop all YARN queues. If > yarn-site's yarn.resourcemanager.work-preserving-recovery.enabled is set to > true, then you can skip this step since the clients will retry on their own.", > "custom_command_name": > "org.apache.ambari.server.serveraction.upgrades.ManualStageAction", > "end_time": -1, > "error_log": "errors-754.txt", > "exit_code": 0, > "host_name": "os-r6-mkqzcs-c10tom21unsecha-6.novalocal", > "id": 754, > "output_log": "output-754.txt", > "request_id": 61, > "role": "AMBARI_SERVER_ACTION", > "stage_id": 1, > "start_time": 1456227329191, > "status": "HOLDING", > "stderr": "", > "stdout": "", > "structured_out": {} > } > } > ] > } > {code} > > > Diffs > ----- > > > ambari-server/src/main/java/com/google/inject/persist/jpa/AmbariJpaPersistModule.java > 604546c > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessor.java > 7f69a31 > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java > 3f4ffeb > > ambari-server/src/main/java/org/apache/ambari/server/orm/AmbariJpaLocalTxnInterceptor.java > 3c953ca > > ambari-server/src/main/java/org/apache/ambari/server/orm/TransactionalLockInterceptor.java > 0cf73cb > > Diff: https://reviews.apache.org/r/43967/diff/ > > > Testing > ------- > > Pending unit tests... > > > Thanks, > > Jonathan Hurley > >
