-
Notifications
You must be signed in to change notification settings - Fork 225
Dns prefetch #336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dns prefetch #336
Conversation
| private final String ucServer; | ||
| private Map<ZoneIndex, ZoneInfo> zones = new ConcurrentHashMap<>(); | ||
| private Client client = new Client(); | ||
| private ZoneInfo zoneInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 这变量好像一直在被赋值而没有用到过?
| @@ -1,5 +1,7 @@ | |||
| package com.qiniu.android.common; | |||
|
|
|||
| import android.util.Log; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 感觉增加了一个没意义的 import
| dnsPrefetcher = new DnsPrefetcher(); | ||
| synchronized (dnsPrefetcher) { | ||
| if (dnsPrefetcher == null) { | ||
| dnsPrefetcher = new DnsPrefetcher(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 为什么这里赋值了两次?一次是 synchronized 一次还不是?
| public static DnsPrefetcher dnsPrefetcher = null; | ||
| private String token; | ||
|
|
||
| private HashMap<String, List<InetAddress>> mConcurrentHashMap = new HashMap<String, List<InetAddress>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 我好像记得我说过这个变量应该是 static 的?
| public void preFetch() throws UnknownHostException { | ||
| for (int i = 0; i < mHosts.size(); i++) { | ||
| List<InetAddress> inetAddresses = getDnsBySystem().lookup(mHosts.get(i)); | ||
| mConcurrentHashMap.put(mHosts.get(i), inetAddresses); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 这里不能因为发生了 UnknownHostException 就退出的情况,发生任何异常都是预期的(不只是 UnknownHostException 吧)要全部 lookup 完才行
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 这里不能因为发生了
UnknownHostException就退出的情况,发生任何异常都是预期的(不只是 UnknownHostException 吧)要全部 lookup 完才行
异常抛出我再全部重新过一遍
| } | ||
|
|
||
| public List<InetAddress> getInetAddressByHost(String host) { | ||
| if (mConcurrentHashMap != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 这变量不是 init 的时候初始化的吗?为什么会有 null 的可能性?
| while (ias.hasMoreElements()) { | ||
| ia = ias.nextElement(); | ||
| if (ia instanceof Inet6Address) { | ||
| continue;// skip ipv6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 考虑下 ipv6 如何兼容,不要 skip 掉,接下来 ipv6 的使用场景会越来越多的。
| } | ||
|
|
||
| private static class ResponseTag { | ||
| public static class ResponseTag { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 这个真的需要 public 出来吗?
| mHosts.add(zone.upDomainsList.get(i)); | ||
| } | ||
| } | ||
| mHosts.add(Config.preQueryHost); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里你确定不需要去重吗?
| } | ||
|
|
||
|
|
||
| public void preHosts() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 这个接口应该是 private 的吧
library/src/main/java/com/qiniu/android/http/DnsPrefetcher.java
Outdated
Show resolved
Hide resolved
| if (builder.useHttps) { | ||
| autoZone = new AutoZone(); | ||
| } else { | ||
| autoZone = new AutoZone(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 你这里直接写 new AutoZone(builder.useHttps) 不就好了?
| public static DnsPrefetcher dnsPrefetcher = null; | ||
| private static String token; | ||
|
|
||
| private static HashMap<String, List<InetAddress>> mConcurrentHashMap = new HashMap<String, List<InetAddress>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 这里的 mConcurrentHashMap 不应该是一个本身就是线程安全的 Map 吗?为什么只使用普通的 HashMap?这样的话任何对 mConcurrentHashMap 的操作都应该加锁了?
| ConcurrentHashMap<String, List<InetAddress>> concurrentHashMap = dnsPrefetcher.getConcurrentHashMap(); | ||
| byte[] dnscache = StringUtils.toByteArray(concurrentHashMap); | ||
|
|
||
| recorder.set("lastcache", data.getBytes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
写2个文件容易有不一致性问题,可以考虑是否只有一个文件,文件名称为第一个文件内容;每次设置新的,都删除旧的,在固定目录保留有一个文件,如果旧文件删除失败,至多有2个文件,对比一下时间戳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
写2个文件容易有不一致性问题,可以考虑是否只有一个文件,文件名称为第一个文件内容;每次设置新的,都删除旧的,在固定目录保留有一个文件,如果旧文件删除失败,至多有2个文件,对比一下时间戳
我优化一下
| e.printStackTrace(); | ||
| } | ||
| if (config.dns != null) { | ||
| DnsPrefetcher.getDnsPrefetcher().dnsPreByCustom(config.dns); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果用户配置了dns会做2次域名解析?一次init系统dns,一次用户dns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果用户配置了dns会做2次域名解析?一次init系统dns,一次用户dns
这里目前是的,这个地方可能考虑了很久会有一点复杂,系统dns是一个必备或者保险操作,如果用户自定义dns预取全部失败的话,起码通过系统dns解析结果还是能正常上传。
同时这里如果只做系统dns预取或者客户自定义dns预取,会在自定义预取时还是需要先获取所有待预取hosts和传token,操作上也没有简化很多。所以在init的时候做了系统预取
| * | ||
| * @return true:重新预期并缓存, false:不需要重新预取和缓存 | ||
| */ | ||
| public static boolean checkRePrefetchDns(String token, Configuration config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个方法有文件的读写,调用之前要确保文件并发读写的问题
| e.printStackTrace(); | ||
| } | ||
| return okhttp3.Dns.SYSTEM.lookup(hostname); | ||
| } else if (DnsPrefetcher.getDnsPrefetcher().getInetAddressByHost(hostname) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不优先使用DnsPrefetcher嘛
|
|
||
| public final class Constants { | ||
| public static final String VERSION = "7.3.15"; | ||
| public static final String VERSION = "7.3.18"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 这个版本不合适 叫 7.4.0 吧
| String currentTime = String.valueOf(System.currentTimeMillis()); | ||
| String localip = AndroidNetwork.getHostIP(); | ||
| String akAndScope = StringUtils.getAkAndScope(token); | ||
| String cacheKey = format(Locale.ENGLISH, "%s:%s:%s", currentTime, localip, akAndScope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 这里建议你把 CacheKey 单独作为一个类,实现 序列化和反序列化接口,否则代码中到处都是针对 ":" 的处理,难以理解
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| byte[] data = recorder.get(recorder.getFileName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 考虑 getFileName() 返回 null 的情况
| e.printStackTrace(); | ||
| } | ||
| byte[] data = recorder.get(recorder.getFileName()); | ||
| DnsPrefetcher.recoverDnsCache(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JemyCheung 考虑 data 返回 null 的情况
| singleDomainRetry = 1; | ||
| retried += 1; | ||
| retried.getAndAdd(1); | ||
| upHost = config.zone.upHost(token.token, config.useHttps, upHost); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里把 singleDomainRetry 和 upHost 也改成 atomic 变量,synchronized 就可以移除了。
In order to resolve DNS resolution errors