Re: svn commit: r583087 - in /incubator/cxf/trunk: integration/jca/src/main/java/org/apache/cxf/jca/cxf/ integration/jca/src/main/java/org/apache/cxf/jca/servant/ integration/jca/src/test/java/org/apache/cxf/jca/servant/ rt/transports/http-jetty/src/main/j...

["Jeff.Yu" <je...@iona.com>]

Hi, Glen

See my comments inline.

Thanks
Jeff

Glen Mazza wrote:
> Am Dienstag, den 09.10.2007, 09:29 +0000 schrieb ningjiang@apache.org:
>
>   
>> Author: ningjiang
>> Date: Tue Oct  9 02:29:43 2007
>> New Revision: 583087  
>>
>> Added: incubator/cxf/trunk/integration/jca/src/main/java/org/apache/cxf/jca/cxf/WorkManagerThreadPool.java
>> URL: http://svn.apache.org/viewvc/incubator/cxf/trunk/integration/jca/src/main/java/org/apache/cxf/jca/cxf/WorkManagerThreadPool.java?rev=583087&view=auto
>> ==============================================================================
>> --- incubator/cxf/trunk/integration/jca/src/main/java/org/apache/cxf/jca/cxf/WorkManagerThreadPool.java (added)
>> +++ incubator/cxf/trunk/integration/jca/src/main/java/org/apache/cxf/jca/cxf/WorkManagerThreadPool.java Tue Oct  9 02:29:43 2007
>> +    
>> +    public void setIsLowOnThreads(boolean isLow) {
>> +        this.isLowOnThreads = isLow;
>> +    }
>> +    
>>     
>
> I'm unsure about the business logic--but should we actually have such a public method like this?  Wouldn't it be the role of the WorkManagerThreadPool itself to determine whether or not it is low on threads?  
>
>
>   
Here is the logic, firstly I am assuming the lowOnThreads is false, and 
then we might got the "START_TIMEOUT" error. If we got this error, we 
know the lowOnThreads is true, so we will set the lowOnThreads true, and 
retry it again.
The WorkManagerThreadPool is actually just a adapter, it is the 
Application server take charges of Thread Pool. But the JCA 1.5 
specification doesn't have this isLowOnThreads() API, so we can't this 
directly from the Application Server.

>> URL: http://svn.apache.org/viewvc/incubator/cxf/trunk/integration/jca/src/main/java/org/apache/cxf/jca/servant/EJBEndpoint.java?rev=583087&r1=583086&r2=583087&view=diff
>> ==============================================================================
>> --- incubator/cxf/trunk/integration/jca/src/main/java/org/apache/cxf/jca/servant/EJBEndpoint.java (original)
>> +++ incubator/cxf/trunk/integration/jca/src/main/java/org/apache/cxf/jca/servant/EJBEndpoint.java Tue Oct  9 02:29:43 2007
>>     
>
>   
>>      public String getServiceClassName() throws Exception {
>> @@ -95,6 +130,18 @@
>>          return "http://" + hostName + ":9999";
>>      }
>>      
>>     
>
> Just to confirm, the port # is *not* configurable, correct?  (i.e., it
> will always be 9999 so no more logic is needed here?)
>
>
>   

It is configurable, but not the port, it is the baseUrl like 
"http://localhost:8080/services" etc in the ra.xml, above is the default 
url, if you don't configure it, we will provide a default one.

>> +    public int getAddressPort(String address) {
>> +        int index = address.lastIndexOf(":");
>> +        int end = address.lastIndexOf("/");
>> +        if (index == 4) {
>> +            return 80;
>> +        }
>>     
>
>
> What about https: (index==5), should this method return 443?
>
>
>   
Good point, I miss this test case. Thanks. ;-)

>> +        if (end < index) {
>> +           return new Integer(address.substring(index +
>> 1)).intValue();
>> +        } 
>> +        return new Integer(address.substring(index + 1,
>> end)).intValue();
>>     
>
>
>   
>> Added:
>> incubator/cxf/trunk/integration/jca/src/test/java/org/apache/cxf/jca/servant/EJBEndpointTest.java
>> URL:
>> http://svn.apache.org/viewvc/incubator/cxf/trunk/integration/jca/src/test/java/org/apache/cxf/jca/servant/EJBEndpointTest.java?rev=583087&view=auto
>> ==============================================================================
>> +/**
>> + * 
>> + */
>> +public class EJBEndpointTest extends Assert {
>> +    
>> +    private EJBEndpoint endpoint;
>> +    
>> +    @Before
>> +    public void setUp() throws Exception {
>> +        endpoint = new EJBEndpoint(null);
>> +    }
>> +    
>> +    @Test
>> +    public void testGetAddressPort() throws Exception {
>> +        int port = endpoint.getAddressPort("http://localhost:8080/services");
>> +        assertEquals(8080, port);
>> +    }
>> +    
>> +    @Test  
>> +    public void testGetAddress80Port() throws Exception {
>> +        int port = endpoint.getAddressPort("http://localhost/services");
>> +        assertEquals(80, port);
>> +    }
>> +    
>> +    @Test
>> +    public void testGetAddressEndPort() throws Exception {
>> +        int port = endpoint.getAddressPort("http://localhost:9999");
>> +        assertEquals(9999, port);
>> +    }
>>     
>
> Depending on your answer above, we may need to add a test for https://
> to make sure it works as well.
>
>
>
>   
>> Modified: incubator/cxf/trunk/rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java
>> URL: http://svn.apache.org/viewvc/incubator/cxf/trunk/rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java?rev=583087&r1=583086&r2=583087&view=diff
>> ==============================================================================
>> --- incubator/cxf/trunk/rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java (original)
>> +++ incubator/cxf/trunk/rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java Tue Oct  9 02:29:43 2007
>> @@ -166,7 +166,7 @@
>>       */
>>      public void shutdown() {
>>          if (shouldDestroyPort()) {
>> -            if (servantCount == 0) {
>> +            if (factory != null && servantCount == 0) {
>>                  factory.destroyForPort(port);
>>              } else {
>>                  LOG.log(Level.WARNING, "FAILED_TO_SHOWDOWN_ENGINE_MSG", port);
>>
>>     
>
> FAILED_TO_SHUTDOWN_ENGINE_MSG
>
> Regards,
> Glen
>
>
>