Dns Codec updated pull request #1622

mbakkar · 2013-07-20T16:59:04Z

I had already closed the other pull request and deleted the repository. I don't think I can reopen pull requests. But here is an updated one (with some changes to work with 4.0.2).

I noticed when writing to a channel information isn't flushed automatically, I have to use writeAndFlush(). Intentional?

ghost · 2013-07-20T17:16:00Z

Build result for #1622 at ef5c715: Failure

normanmaurer · 2013-07-20T17:16:45Z

Yes... See the release notes of 4.0.0.Final.

Will check later

Am 20.07.2013 um 18:59 schrieb mbakkar [email protected]:

I had already closed the other pull request and deleted the repository. I don't think I can reopen pull requests. But here is an updated one (with some changes to work with 4.0.2).

I noticed when writing to a channel information isn't flushed automatically, I have to use writeAndFlush(). Intentional?

You can merge this Pull Request by running

git pull https://github.com/mbakkar/netty-1 master
Or view, comment on, or merge it at:

#1622

Commit Summary

Updated for netty 4.0.2
File Changes

A codec/src/main/java/io/netty/handler/codec/dns/DnsEntry.java (381)
A codec/src/main/java/io/netty/handler/codec/dns/DnsHeader.java (175)
A codec/src/main/java/io/netty/handler/codec/dns/DnsMessage.java (132)
A codec/src/main/java/io/netty/handler/codec/dns/DnsQuery.java (31)
A codec/src/main/java/io/netty/handler/codec/dns/DnsQueryEncoder.java (109)
A codec/src/main/java/io/netty/handler/codec/dns/DnsQueryHeader.java (65)
A codec/src/main/java/io/netty/handler/codec/dns/DnsResponse.java (87)
A codec/src/main/java/io/netty/handler/codec/dns/DnsResponseDecoder.java (211)
A codec/src/main/java/io/netty/handler/codec/dns/DnsResponseHeader.java (242)
A codec/src/main/java/io/netty/handler/codec/dns/Question.java (71)
A codec/src/main/java/io/netty/handler/codec/dns/Resource.java (128)
A codec/src/main/java/io/netty/handler/codec/dns/ResponseCode.java (125)
A codec/src/main/java/io/netty/handler/codec/dns/package-info.java (21)
A codec/src/test/java/io/netty/handler/codec/dns/DnsCodecTest.java (105)
A codec/src/test/java/io/netty/handler/codec/dns/package-info.java (21)
A handler/src/main/java/io/netty/handler/dns/DnsCallback.java (220)
A handler/src/main/java/io/netty/handler/dns/DnsClientInitializer.java (37)
A handler/src/main/java/io/netty/handler/dns/DnsExchangeFactory.java (494)
A handler/src/main/java/io/netty/handler/dns/InboundDnsMessageHandler.java (53)
A handler/src/main/java/io/netty/handler/dns/ResourceCache.java (172)
A handler/src/main/java/io/netty/handler/dns/SingleResultCallback.java (59)
A handler/src/main/java/io/netty/handler/dns/decoder/AddressDecoder.java (64)
A handler/src/main/java/io/netty/handler/dns/decoder/DomainDecoder.java (42)
A handler/src/main/java/io/netty/handler/dns/decoder/MailExchangerDecoder.java (47)
A handler/src/main/java/io/netty/handler/dns/decoder/RecordDecoder.java (45)
A handler/src/main/java/io/netty/handler/dns/decoder/RecordDecoderFactory.java (135)
A handler/src/main/java/io/netty/handler/dns/decoder/ServiceDecoder.java (49)
A handler/src/main/java/io/netty/handler/dns/decoder/StartOfAuthorityDecoder.java (52)
A handler/src/main/java/io/netty/handler/dns/decoder/TextDecoder.java (54)
A handler/src/main/java/io/netty/handler/dns/decoder/package-info.java (21)
A handler/src/main/java/io/netty/handler/dns/decoder/record/MailExchangerRecord.java (57)
A handler/src/main/java/io/netty/handler/dns/decoder/record/ServiceRecord.java (113)
A handler/src/main/java/io/netty/handler/dns/decoder/record/StartOfAuthorityRecord.java (123)
A handler/src/main/java/io/netty/handler/dns/decoder/record/package-info.java (21)
A handler/src/main/java/io/netty/handler/dns/package-info.java (21)
A handler/src/test/java/io/netty/handler/dns/DnsImplementationTest.java (85)
A handler/src/test/java/io/netty/handler/dns/package-info.java (21)
Patch Links:

https://github.com/netty/netty/pull/1622.patch
https://github.com/netty/netty/pull/1622.diff

caniszczyk · 2013-07-20T18:13:02Z

We need to make sure @mbakkar signs a CLA too if that wasn't done already :)

Really looking forward to getting this in by the end of the summer :)

mbakkar · 2013-07-20T18:58:44Z

I believe I did sign a CLA in the beginning

ghost · 2013-07-20T19:12:41Z

Build result for #1622 at f16cd9b: Success

normanmaurer · 2013-07-20T19:25:04Z

@caniszczyk let me review this and merge in at the end...

normanmaurer · 2013-07-20T19:35:03Z

codec/src/main/java/io/netty/handler/codec/dns/DnsResponse.java

+     */
+    @Override
+    public ByteBuf content() {
+        return rawPacket.readerIndex(originalIndex);


Why reset the index here ? I think this should not be done as we not do this in other ByteBufHolder impls

-DnsResponse returns content with unmodified index

normanmaurer · 2013-07-20T19:50:28Z

@mbakkar what I would like to see is to create a new "module" called codec-dns and merge everything you have currently split into codec and handler into it.

- Decoding and encoding methods now pass ByteBufAllocator and Charset

mbakkar · 2013-07-20T20:14:32Z

I have to retain it because the DnsResponse is used in the next handler in the chain (InboundDnsMessageHandler for the impl test, and for the codec test it is the "Handler" inner-class in DnsCodecTest)

EDIT: some package-infos got erased, re-committing

ghost · 2013-07-20T20:25:15Z

Build result for #1622 at 17e3bb7: Success

normanmaurer · 2013-07-21T08:55:37Z

codec-dns/src/main/java/io/netty/handler/dns/DnsExchangeFactory.java

+            Callable<List<ByteBuf>> callback = new DnsCallback<List<ByteBuf>>(-1, sendQuery(DnsEntry.TYPE_A,
+                    "google.com", id, channel));
+            return callback.call() != null;
+        } catch (Exception e) {


Don't swallow exceptions... Please at least log it.

- RecordDecoderFactory throws DecoderException now in constructor

ghost · 2013-07-21T16:59:11Z

Build result for #1622 at 0e4bb81: Success

normanmaurer · 2013-07-21T18:00:13Z

@trustin please also review..

ghost · 2013-08-11T21:08:46Z

Build result for #1622 at 562767b: Failure

ghost · 2013-08-11T21:21:38Z

Build result for #1622 at 218a65d: Failure

ghost · 2013-08-11T21:32:17Z

Build result for #1622 at 6b8b35e: Failure

mbakkar · 2013-08-16T15:40:17Z

Doesn't it need to use its resolver in Bootstrap classes?

trustin · 2013-08-16T21:53:44Z

Not really. The resolver interface should reside at netty-transport. netty-dns should provide only the async resolver implementation.

trustin · 2013-08-16T21:58:13Z

I guess this could clear things a lot:

netty-transport:
public interface io.netty.dns.DomainNameResolver

netty-asyncdns:
public class io.netty.dns.async.AsyncDomainNameResolver implements io.netty.dns.DomainNameResolver

i.e. io.netty.dns belongs to netty-transport. io.netty.dns.async belongs to netty-asyncdns.

mbakkar · 2013-08-16T23:28:09Z

I see, and AsyncDomainNameResolver would have an instance of/extend DnsResolver, good idea.

normanmaurer · 2013-08-19T10:54:21Z

codec-dns/src/main/java/io/netty/handler/codec/dns/DnsResponseDecoder.java

+            response.addQuestion(decodeQuestion(buf));
+        }
+        if (header.getResponseCode() != 0) {
+            System.err.println("Encountered error decoding DNS response for domain \""


I think you should just use "return response" here.

travishaagen · 2013-08-19T15:22:07Z

I see that DnsResource provides access to the TTL value, but the decoders in io.netty.dns.decoder do not expose the TTL. For some kinds of DNS clients, we might need the TTL, to provide a secondary caching layer (e.g., Sender Policy Framework).

If this is an uncommon need, then one could extend all of the decoders for their particular use-case, but if clients will often need to know the TTL, then perhaps it should be copied from DnsResource within the decode-function and exposed by the decoders.

caniszczyk · 2013-09-03T13:42:51Z

@mbakkar how are things going with this project?

mbakkar · 2013-09-04T02:24:58Z

Hi just a brief update.. I haven't really made progress on this in the past 2 weeks. I know clearly what I need to do but I've been stretched for time since I started school. I finish handing in a couple major assignments tomorrow at 11:59 PM, and then I should have a few days leeway where I can continue working on this after that. Sorry for getting back so late..!

caniszczyk · 2013-09-09T16:16:05Z

Thanks @mbakkar! Pencils down starts next week so it would be good to start aiming for the finish line on the project.

@trustin is in the main Twitter office this week so we can hopefully work on getting the code in a state that it can be merged into master :)

trustin · 2013-09-09T17:29:37Z

@mbakkar, no worries! You've done a great job so far. Please take your time.

@caniszczyk, sure. I'll review it and merge it into master once it's ready. :-)

normanmaurer · 2013-11-05T05:22:00Z

@mbakkar @caniszczyk @trustin working on clean this up now and merge to master.. stay tuned.

normanmaurer · 2013-11-05T05:52:53Z

I merged everything into dns branch.. Let me polish the code there. So closing this now.. @mbakkar thanks again for all your work

This PR also includes a AsynchronousDnsResolver which allows to resolve DNS entries in a non blocking way by make use of the dns codec and netty transport itself.

@mbakkar

Motivation: As part of GSOC 2013 we had @mbakkar working on a DNS codec but did not integrate it yet as it needs some cleanup. This commit is based on @mbakkar's work and provide the codec for DNS. Modifications: Add DNS codec Result: Reusable DNS codec will be included in netty. This PR also includes a AsynchronousDnsResolver which allows to resolve DNS entries in a non blocking way by make use of the dns codec and netty transport itself.

@mbakkar

Motivation: As part of GSOC 2013 we had @mbakkar working on a DNS codec but did not integrate it yet as it needs some cleanup. This commit is based on @mbakkar's work and provide the codec for DNS. Modifications: Add DNS codec Result: Reusable DNS codec will be included in netty. This PR also includes a AsynchronousDnsResolver which allows to resolve DNS entries in a non blocking way by make use of the dns codec and netty transport itself.

@mbakkar

Motivation: As part of GSOC 2013 we had @mbakkar working on a DNS codec but did not integrate it yet as it needs some cleanup. This commit is based on @mbakkar's work and provide the codec for DNS. Modifications: Add DNS codec Result: Reusable DNS codec will be included in netty. This PR also includes a AsynchronousDnsResolver which allows to resolve DNS entries in a non blocking way by make use of the dns codec and netty transport itself.

Updated for netty 4.0.2

ef5c715

Changed write to writeAndFlush in DnsCodecTest

f16cd9b

ghost assigned normanmaurer Jul 20, 2013

normanmaurer reviewed Jul 20, 2013
View reviewed changes

-Replaced Charset.forName with CharsetUtil.UTF_8

c47ac23

-DnsResponse returns content with unmodified index

- Moved dns to its own module "codec-dns"

2248a5f

- Decoding and encoding methods now pass ByteBufAllocator and Charset

Added package-infos that were accidentally deleted

17e3bb7

normanmaurer reviewed Jul 21, 2013
View reviewed changes

- Added more logging, removed printing of exceptions

0e4bb81

- RecordDecoderFactory throws DecoderException now in constructor

Small logging changes

89377aa

Transport dependency

562767b

Cyclical references? Maybe it's time to start using Maven.

218a65d

.

6b8b35e

ghost mentioned this pull request Aug 14, 2013

circular-dependency refactorings #1740

Closed

normanmaurer reviewed Aug 19, 2013
View reviewed changes

normanmaurer closed this Nov 5, 2013

This was referenced Nov 8, 2013

WS.post blocking on DNS issues (play-2.2.1 scala] playframework/playframework#1997

Closed

Add non-blocking DNS resolution spray/spray#73

Closed

jrudolph mentioned this pull request Feb 10, 2015

Add non-blocking DNS resolution akka/akka#16846

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dns Codec updated pull request #1622

Dns Codec updated pull request #1622

mbakkar commented Jul 20, 2013

ghost commented Jul 20, 2013

normanmaurer commented Jul 20, 2013

caniszczyk commented Jul 20, 2013

mbakkar commented Jul 20, 2013

ghost commented Jul 20, 2013

normanmaurer commented Jul 20, 2013

normanmaurer Jul 20, 2013

normanmaurer commented Jul 20, 2013

mbakkar commented Jul 20, 2013

ghost commented Jul 20, 2013

normanmaurer Jul 21, 2013

ghost commented Jul 21, 2013

normanmaurer commented Jul 21, 2013

ghost commented Aug 11, 2013

ghost commented Aug 11, 2013

ghost commented Aug 11, 2013

mbakkar commented Aug 16, 2013

trustin commented Aug 16, 2013

trustin commented Aug 16, 2013

mbakkar commented Aug 16, 2013

normanmaurer Aug 19, 2013

travishaagen commented Aug 19, 2013

caniszczyk commented Sep 3, 2013

mbakkar commented Sep 4, 2013

caniszczyk commented Sep 9, 2013

trustin commented Sep 9, 2013

normanmaurer commented Nov 5, 2013

normanmaurer commented Nov 5, 2013

Dns Codec updated pull request #1622

Dns Codec updated pull request #1622

Conversation

mbakkar commented Jul 20, 2013

ghost commented Jul 20, 2013

normanmaurer commented Jul 20, 2013

caniszczyk commented Jul 20, 2013

mbakkar commented Jul 20, 2013

ghost commented Jul 20, 2013

normanmaurer commented Jul 20, 2013

normanmaurer Jul 20, 2013

Choose a reason for hiding this comment

normanmaurer commented Jul 20, 2013

mbakkar commented Jul 20, 2013

ghost commented Jul 20, 2013

normanmaurer Jul 21, 2013

Choose a reason for hiding this comment

ghost commented Jul 21, 2013

normanmaurer commented Jul 21, 2013

ghost commented Aug 11, 2013

ghost commented Aug 11, 2013

ghost commented Aug 11, 2013

mbakkar commented Aug 16, 2013

trustin commented Aug 16, 2013

trustin commented Aug 16, 2013

mbakkar commented Aug 16, 2013

normanmaurer Aug 19, 2013

Choose a reason for hiding this comment

travishaagen commented Aug 19, 2013

caniszczyk commented Sep 3, 2013

mbakkar commented Sep 4, 2013

caniszczyk commented Sep 9, 2013

trustin commented Sep 9, 2013

normanmaurer commented Nov 5, 2013

normanmaurer commented Nov 5, 2013