> On May 14, 2015, 10:21 a.m., Rajini Sivaram wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > line 190 > > <https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line190> > > > > I think when delegated tasks are run asynchronously, selection > > interestOps should be managed within the transport layer. Otherwise, > > packets may be processed out of order. InterestOps should be set to zero > > before tasks are executed, and changes to interestOps due to packets being > > ready for transmission etc. should be cached and set after the delegated > > tasks are executed. Not sure if the current transport layer interface is > > sufficient to handle this correctly. > > Sriharsha Chintalapani wrote: > I am not sure how packets can be processed out of order here. If the > handshakeStatus is NEED_TASK I call tasks() method which hands over any > delegated tasks to executorService . As long as these tasks are not finished > handshakeStatus stays NEED_TASK only after these tasks are finished we are > resuming handshake. There is no further reading/writing to socketChannel > until these tasks are finished. Can you elaborate on why you need InterestOps > to set to 0.
I think what happens at the moment is that handshake with a delegated task doesn't change interestOps. So it results in a tight loop when interestOps has write enabled. In this case, tasks() is called over and over again when poll() returns, until the delegated task is complete. This is undesirable, but doesn't lead to errors. In the case of delegated tasks in read/write, I am not sure the code checks for NEED_TASK upfront, so it is not clear if they dont lead to packets being processed out of order because of interestOps not being reset. Basically we want to avoid processing any packets while potentially long running delegated tasks are being processed on a different thread. It feels like this is being achieved with a tight polling loop at the moment, which negates the benefit of running these tasks in a different thread. Setting interestOps to zero and resetting them when the task is complete would avoid the polling overhead and simplify the code in read/write which otherwise need to check if another thread is handling a delegated task. > On May 14, 2015, 10:21 a.m., Rajini Sivaram wrote: > > clients/src/main/java/org/apache/kafka/common/network/Authenticator.java, > > line 47 > > <https://reviews.apache.org/r/33620/diff/5/?file=957060#file957060line47> > > > > Don't think authenticator interface should handle selection > > interestOps. Needs better encapsulation. > > Sriharsha Chintalapani wrote: > SaslAuth works by reading socketChannel and doing accept token on > incoming data on server side and similarly saslclient sends a new token . Why > do you think authenticate shouldn't handle interestOps since its reading and > writing to socketChannel. Whats your proposal > > Rajini Sivaram wrote: > At the moment management of interestOps is spread across Selector, > TransportLayer and Authenticator. I would have hoped that they could be > contained within the transport layer. My concern is that with transport layer > setting interestOps from multiple threads (for delegated tasks in SSL), it > may be too messy to handle interestOps within a pluggable authenticator as > well. Maybe it is better to address this when SaslAuth implementation is > ready. > > Sriharsha Chintalapani wrote: > Only selector is the one setting the interestOps not transportLayer or > authenticator , they just return a interstOp. How is multiple threads an > issue here? May not be an issue if re-negotiation and full graceful shutdown are not required. Will wait to see how interestOps are handled for delegated tasks (in the other issue raised) before closing this one. - Rajini ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review83741 ----------------------------------------------------------- On May 12, 2015, 11:20 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33620/ > ----------------------------------------------------------- > > (Updated May 12, 2015, 11:20 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1690 > https://issues.apache.org/jira/browse/KAFKA-1690 > > > Repository: kafka > > > Description > ------- > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. SSLFactory tests. > > > KAFKA-1690. new java producer needs ssl support as a client. Added > PrincipalBuilder. > > > Diffs > ----- > > build.gradle fef515b3b2276b1f861e7cc2e33e74c3ce5e405b > checkstyle/checkstyle.xml a215ff36e9252879f1e0be5a86fef9a875bb8f38 > checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03 > clients/src/main/java/org/apache/kafka/clients/ClientUtils.java > 0d68bf1e1e90fe9d5d4397ddf817b9a9af8d9f7a > clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java > cf32e4e7c40738fe6d8adc36ae0cfad459ac5b0b > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java > bdff518b732105823058e6182f445248b45dc388 > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > d301be4709f7b112e1f3a39f3c04cfa65f00fa60 > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 8e336a3aa96c73f52beaeb56b931baf4b026cf21 > clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java > 187d0004c8c46b6664ddaffecc6166d4b47351e5 > clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java > c4fa058692f50abb4f47bd344119d805c60123f5 > clients/src/main/java/org/apache/kafka/common/network/Authenticator.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/Channel.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/DefaultAuthenticator.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/Selectable.java > b5f8d83e89f9026dc0853e5f92c00b2d7f043e22 > clients/src/main/java/org/apache/kafka/common/network/Selector.java > 57de0585e5e9a53eb9dcd99cac1ab3eb2086a302 > clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java > dab1a94dd29563688b6ecf4eeb0e180b06049d3f > > clients/src/main/java/org/apache/kafka/common/security/auth/DefaultPrincipalBuilder.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/security/auth/KafkaPrincipal.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/utils/Utils.java > f73eedb030987f018d8446bb1dcd98d19fa97331 > clients/src/test/java/org/apache/kafka/common/network/EchoServer.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SSLFactoryTest.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java > d5b306b026e788b4e5479f3419805aa49ae889f3 > clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java > 2ebe3c21f611dc133a2dbb8c7dfb0845f8c21498 > clients/src/test/java/org/apache/kafka/test/TestSSLUtils.java PRE-CREATION > > Diff: https://reviews.apache.org/r/33620/diff/ > > > Testing > ------- > > > Thanks, > > Sriharsha Chintalapani > >
