Re: Review Request 55178: Investigate Changing the Default Container Policy in JPA From Vector to ArrayList

2017-01-04 Thread Sebastian Toader

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


Ship it!




Ship It!

- Sebastian Toader


On Jan. 4, 2017, 5:56 p.m., Jonathan Hurley wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55178/
> ---
> 
> (Updated Jan. 4, 2017, 5:56 p.m.)
> 
> 
> Review request for Ambari, Nate Cole, Sebastian Toader, and Sid Wagle.
> 
> 
> Bugs: AMBARI-19364
> https://issues.apache.org/jira/browse/AMBARI-19364
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Recently I noticed that collections coming back from JPA were using a 
> {{Vector}} as their concrete collection. This seems very wrong as {{Vector}} 
> is a completely synchronized collection. 
> 
> Tracing through EclipseLink, it seems like this really only affects 
> [ReadAllQuery|http://grepcode.com/file/repo1.maven.org/maven2/org.eclipse.persistence/eclipselink/2.5.2/org/eclipse/persistence/queries/ReadAllQuery.java/#68].
>  Essentially, EclipseLink uses  a 
> [ContainerPolicy|http://grepcode.com/file/repo1.maven.org/maven2/org.eclipse.persistence/eclipselink/2.5.2/org/eclipse/persistence/internal/queries/ContainerPolicy.java#ContainerPolicy]
>  to determine how the result set should be collected. There are policies for 
> {{Vector}}, {{ArrayList}}, {{HashSet}}, etc. 
> 
> The interesting part here is that this really only affects the 
> {{ReadAllQuery}} which is used by JPA's {{Query#getResultList()}} ... anytime 
> it needs to create a collection of entities, it's going to use a {{Vector}}.
> 
> There's actually very little documentation on this. In fact, the only way to 
> change this is either set a global value for the default container policy, or 
> set a per-query policy. An example of setting the global policy would be:
> 
> {code}
> public class EclipseLinkSessionCustomizer implements SessionCustomizer {
> 
>   /**
>* {@inheritDoc}
>* 
>* This class exists for quick customization purposes.
>*/
>   @Override
>   public void customize(Session session) throws Exception {
> // ensure db behavior is same as shared cache
> DatabaseLogin databaseLogin = (DatabaseLogin) 
> session.getDatasourceLogin();
> 
> databaseLogin.setTransactionIsolation(DatabaseLogin.TRANSACTION_READ_COMMITTED);
> 
> // for some reason, read-all queries use a Vector as their container for
> // result items - this seems like an unnecessary performance hit since
> // Vectors are synchronized and there's no apparent reason to provide a
> // thread-safe collection on a read all query
> 
> ContainerPolicy.setDefaultContainerClass(CoreClassConstants.ArrayList_class);
>   }
> }
> {code}
> 
> This indeed causes collections returned by queries to be backed by 
> ArrayLists. 
> 
> There is some discussion on this topic already:
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=255634
> 
> It seems like this was kept as a {{Vector}} purely for backward compatibility 
> reasons.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/EclipseLinkSessionCustomizer.java
>  6717e01 
> 
> Diff: https://reviews.apache.org/r/55178/diff/
> 
> 
> Testing
> ---
> 
> Ran a full deployment and didn't see any issues. Verfied that ArrayList are 
> returned in `ReadAllQuery` instead of Vector.
> 
> 
> Thanks,
> 
> Jonathan Hurley
> 
>



Re: Review Request 55178: Investigate Changing the Default Container Policy in JPA From Vector to ArrayList

2017-01-04 Thread Sid Wagle

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


Ship it!




Ship It!

- Sid Wagle


On Jan. 4, 2017, 4:56 p.m., Jonathan Hurley wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55178/
> ---
> 
> (Updated Jan. 4, 2017, 4:56 p.m.)
> 
> 
> Review request for Ambari, Nate Cole, Sebastian Toader, and Sid Wagle.
> 
> 
> Bugs: AMBARI-19364
> https://issues.apache.org/jira/browse/AMBARI-19364
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Recently I noticed that collections coming back from JPA were using a 
> {{Vector}} as their concrete collection. This seems very wrong as {{Vector}} 
> is a completely synchronized collection. 
> 
> Tracing through EclipseLink, it seems like this really only affects 
> [ReadAllQuery|http://grepcode.com/file/repo1.maven.org/maven2/org.eclipse.persistence/eclipselink/2.5.2/org/eclipse/persistence/queries/ReadAllQuery.java/#68].
>  Essentially, EclipseLink uses  a 
> [ContainerPolicy|http://grepcode.com/file/repo1.maven.org/maven2/org.eclipse.persistence/eclipselink/2.5.2/org/eclipse/persistence/internal/queries/ContainerPolicy.java#ContainerPolicy]
>  to determine how the result set should be collected. There are policies for 
> {{Vector}}, {{ArrayList}}, {{HashSet}}, etc. 
> 
> The interesting part here is that this really only affects the 
> {{ReadAllQuery}} which is used by JPA's {{Query#getResultList()}} ... anytime 
> it needs to create a collection of entities, it's going to use a {{Vector}}.
> 
> There's actually very little documentation on this. In fact, the only way to 
> change this is either set a global value for the default container policy, or 
> set a per-query policy. An example of setting the global policy would be:
> 
> {code}
> public class EclipseLinkSessionCustomizer implements SessionCustomizer {
> 
>   /**
>* {@inheritDoc}
>* 
>* This class exists for quick customization purposes.
>*/
>   @Override
>   public void customize(Session session) throws Exception {
> // ensure db behavior is same as shared cache
> DatabaseLogin databaseLogin = (DatabaseLogin) 
> session.getDatasourceLogin();
> 
> databaseLogin.setTransactionIsolation(DatabaseLogin.TRANSACTION_READ_COMMITTED);
> 
> // for some reason, read-all queries use a Vector as their container for
> // result items - this seems like an unnecessary performance hit since
> // Vectors are synchronized and there's no apparent reason to provide a
> // thread-safe collection on a read all query
> 
> ContainerPolicy.setDefaultContainerClass(CoreClassConstants.ArrayList_class);
>   }
> }
> {code}
> 
> This indeed causes collections returned by queries to be backed by 
> ArrayLists. 
> 
> There is some discussion on this topic already:
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=255634
> 
> It seems like this was kept as a {{Vector}} purely for backward compatibility 
> reasons.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/EclipseLinkSessionCustomizer.java
>  6717e01 
> 
> Diff: https://reviews.apache.org/r/55178/diff/
> 
> 
> Testing
> ---
> 
> Ran a full deployment and didn't see any issues. Verfied that ArrayList are 
> returned in `ReadAllQuery` instead of Vector.
> 
> 
> Thanks,
> 
> Jonathan Hurley
> 
>



Re: Review Request 55178: Investigate Changing the Default Container Policy in JPA From Vector to ArrayList

2017-01-04 Thread Nate Cole

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


Ship it!




Ship It!

- Nate Cole


On Jan. 4, 2017, 11:56 a.m., Jonathan Hurley wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55178/
> ---
> 
> (Updated Jan. 4, 2017, 11:56 a.m.)
> 
> 
> Review request for Ambari, Nate Cole, Sebastian Toader, and Sid Wagle.
> 
> 
> Bugs: AMBARI-19364
> https://issues.apache.org/jira/browse/AMBARI-19364
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Recently I noticed that collections coming back from JPA were using a 
> {{Vector}} as their concrete collection. This seems very wrong as {{Vector}} 
> is a completely synchronized collection. 
> 
> Tracing through EclipseLink, it seems like this really only affects 
> [ReadAllQuery|http://grepcode.com/file/repo1.maven.org/maven2/org.eclipse.persistence/eclipselink/2.5.2/org/eclipse/persistence/queries/ReadAllQuery.java/#68].
>  Essentially, EclipseLink uses  a 
> [ContainerPolicy|http://grepcode.com/file/repo1.maven.org/maven2/org.eclipse.persistence/eclipselink/2.5.2/org/eclipse/persistence/internal/queries/ContainerPolicy.java#ContainerPolicy]
>  to determine how the result set should be collected. There are policies for 
> {{Vector}}, {{ArrayList}}, {{HashSet}}, etc. 
> 
> The interesting part here is that this really only affects the 
> {{ReadAllQuery}} which is used by JPA's {{Query#getResultList()}} ... anytime 
> it needs to create a collection of entities, it's going to use a {{Vector}}.
> 
> There's actually very little documentation on this. In fact, the only way to 
> change this is either set a global value for the default container policy, or 
> set a per-query policy. An example of setting the global policy would be:
> 
> {code}
> public class EclipseLinkSessionCustomizer implements SessionCustomizer {
> 
>   /**
>* {@inheritDoc}
>* 
>* This class exists for quick customization purposes.
>*/
>   @Override
>   public void customize(Session session) throws Exception {
> // ensure db behavior is same as shared cache
> DatabaseLogin databaseLogin = (DatabaseLogin) 
> session.getDatasourceLogin();
> 
> databaseLogin.setTransactionIsolation(DatabaseLogin.TRANSACTION_READ_COMMITTED);
> 
> // for some reason, read-all queries use a Vector as their container for
> // result items - this seems like an unnecessary performance hit since
> // Vectors are synchronized and there's no apparent reason to provide a
> // thread-safe collection on a read all query
> 
> ContainerPolicy.setDefaultContainerClass(CoreClassConstants.ArrayList_class);
>   }
> }
> {code}
> 
> This indeed causes collections returned by queries to be backed by 
> ArrayLists. 
> 
> There is some discussion on this topic already:
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=255634
> 
> It seems like this was kept as a {{Vector}} purely for backward compatibility 
> reasons.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/orm/EclipseLinkSessionCustomizer.java
>  6717e01 
> 
> Diff: https://reviews.apache.org/r/55178/diff/
> 
> 
> Testing
> ---
> 
> Ran a full deployment and didn't see any issues. Verfied that ArrayList are 
> returned in `ReadAllQuery` instead of Vector.
> 
> 
> Thanks,
> 
> Jonathan Hurley
> 
>



Review Request 55178: Investigate Changing the Default Container Policy in JPA From Vector to ArrayList

2017-01-04 Thread Jonathan Hurley

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

Review request for Ambari, Nate Cole, Sebastian Toader, and Sid Wagle.


Bugs: AMBARI-19364
https://issues.apache.org/jira/browse/AMBARI-19364


Repository: ambari


Description
---

Recently I noticed that collections coming back from JPA were using a 
{{Vector}} as their concrete collection. This seems very wrong as {{Vector}} is 
a completely synchronized collection. 

Tracing through EclipseLink, it seems like this really only affects 
[ReadAllQuery|http://grepcode.com/file/repo1.maven.org/maven2/org.eclipse.persistence/eclipselink/2.5.2/org/eclipse/persistence/queries/ReadAllQuery.java/#68].
 Essentially, EclipseLink uses  a 
[ContainerPolicy|http://grepcode.com/file/repo1.maven.org/maven2/org.eclipse.persistence/eclipselink/2.5.2/org/eclipse/persistence/internal/queries/ContainerPolicy.java#ContainerPolicy]
 to determine how the result set should be collected. There are policies for 
{{Vector}}, {{ArrayList}}, {{HashSet}}, etc. 

The interesting part here is that this really only affects the {{ReadAllQuery}} 
which is used by JPA's {{Query#getResultList()}} ... anytime it needs to create 
a collection of entities, it's going to use a {{Vector}}.

There's actually very little documentation on this. In fact, the only way to 
change this is either set a global value for the default container policy, or 
set a per-query policy. An example of setting the global policy would be:

{code}
public class EclipseLinkSessionCustomizer implements SessionCustomizer {

  /**
   * {@inheritDoc}
   * 
   * This class exists for quick customization purposes.
   */
  @Override
  public void customize(Session session) throws Exception {
// ensure db behavior is same as shared cache
DatabaseLogin databaseLogin = (DatabaseLogin) session.getDatasourceLogin();

databaseLogin.setTransactionIsolation(DatabaseLogin.TRANSACTION_READ_COMMITTED);

// for some reason, read-all queries use a Vector as their container for
// result items - this seems like an unnecessary performance hit since
// Vectors are synchronized and there's no apparent reason to provide a
// thread-safe collection on a read all query

ContainerPolicy.setDefaultContainerClass(CoreClassConstants.ArrayList_class);
  }
}
{code}

This indeed causes collections returned by queries to be backed by ArrayLists. 

There is some discussion on this topic already:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=255634

It seems like this was kept as a {{Vector}} purely for backward compatibility 
reasons.


Diffs
-

  
ambari-server/src/main/java/org/apache/ambari/server/orm/EclipseLinkSessionCustomizer.java
 6717e01 

Diff: https://reviews.apache.org/r/55178/diff/


Testing
---

Ran a full deployment and didn't see any issues. Verfied that ArrayList are 
returned in `ReadAllQuery` instead of Vector.


Thanks,

Jonathan Hurley