> On April 11, 2016, 12:36 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertDispatchDAO.java,
> >  lines 701-708
> > <https://reviews.apache.org/r/45904/diff/3/?file=1337530#file1337530line701>
> >
> >     This solution is still far from ideal. You'll be making something like 
> > 20,000 database queries and if even 1 fails, then the entire transaction is 
> > rolled back. 
> >     
> >     This is why we went for a pure JPQL approach here; it was clean and 
> > didn't cause any weird problems.
> >     
> >     This also doesn't address the fact that nothing prevents future JPQL 
> > queries from causing a similar problem.
> >     
> >     Perhaps we need to consider a different approach. The issue was that 
> > MySQL was using temporary tables outside of a transaction. If we can solve 
> > that problem, we can leave the existing JPQL.
> 
> Myroslav Papirkovskyy wrote:
>     This is not quite correct. Issue was that EclipseLink uses MySQL 
> temporary tables INSIDE of transactions.
>     Temp tables outside of transaction are supported even with GTID enabled.
>     
>     However disabling temp tables simply doesn't work. EclipseLink starts to 
> generate invalid queries for MySQL. And after quick look at sources it seems 
> that major modifications are required to fix this.
>     For MySQL temp tables are used to bypass limitations of subqueries in 
> delete/update statements.
> 
> Jonathan Hurley wrote:
>     Ah, that stinks. Yeah, my hope was that telling MySQL to not use temp 
> tables would be the ticket. But if it starts forming bad queries, that's no 
> good either. 
>     
>     This is quite bad in that we really can't leverage JPQL anymore if we say 
> we support GTID. I'm curious if running in the WARN mode would be a good 
> balances. This way, GTID will be enabled, but some Ambari queries which 
> violated GTID constraints can still be allowed to execute. 
>     
>     The current approach seems very fragile and extremely hard to enforce 
> moving forward.

Agree that we would entirely rely on system test to catch any queries in the 
future.
Unclear why you thing the implementation is fragile.


- Sid


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45904/#review128100
-----------------------------------------------------------


On April 8, 2016, 8:56 p.m., Sid Wagle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45904/
> -----------------------------------------------------------
> 
> (Updated April 8, 2016, 8:56 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, Myroslav Papirkovskyy, and Sumit 
> Mohanty.
> 
> 
> Bugs: AMBARI-15774
>     https://issues.apache.org/jira/browse/AMBARI-15774
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Approach:
> 
> - Added member varaibles only when underlying DB columns exist
> - Changed DELETE via implcit join query to find and delete
> - MySQL temp tables are only used for UPDATE ALL and DELETE ALL, so SELECT 
> and DELETE is a safer approach
> - Visually inspected all Entities under 
> org.apache.ambari.server.orm.entities.* for absence of this pattern
> 
> Issue: Ambari upgrade is failing from 2.2.0 to 2.2.1.1 with below error:
> 
> Error output from schema upgrade command: 
> Exception in thread "main" org.apache.ambari.server.AmbariException: 
> Exception [EclipseLink-4002] (Eclipse Persistence Services - 
> 2.5.2.v20140319-9ad6abd): 
> org.eclipse.persistence.exceptions.DatabaseException 
> Internal Exception: 
> com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Table 
> 'ambari.TL_alert_notice' doesn't exist 
> Error Code: 1146
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/events/listeners/alerts/AlertReceivedListener.java
>  a3befa6 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertDefinitionDAO.java
>  82fa48a 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertDispatchDAO.java
>  1f1aa45 
>   ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertsDAO.java 
> 781d4cf 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertCurrentEntity.java
>  604b00e 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertHistoryEntity.java
>  03ffcde 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertNoticeEntity.java
>  ae7495d 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertResourceProviderTest.java
>  d611fe8 
>   
> ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertDefinitionDAOTest.java
>  36e75e7 
>   
> ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertsDAOTest.java
>  10f099e 
> 
> Diff: https://reviews.apache.org/r/45904/diff/
> 
> 
> Testing
> -------
> 
> Manually verified on 1 node repro cluster.
> 
> All alert unit test passed.
> 
> mvn clean test -Dtest=Alert* -Drat.ignoreErrors -DfailIfNoTests=false
> 
> 
> Thanks,
> 
> Sid Wagle
> 
>

Reply via email to