P2p outbound #515
P2p outbound #515
Conversation
Improve port binding and db error msgs
AionJayT
requested review from
qoire,
AlexandraRoatis,
iamyulong and
aion-sridevi
| private final int maxActiveNodes; | ||
|
|
||
| private final int maxTempNodes; | ||
|
|
||
| private static final Random random = new SecureRandom(ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array()); |
There was a problem hiding this comment.
What is the rationale behind seeding the secure random? My opinion is that our seed is less secure than if random came from a random source.
There was a problem hiding this comment.
the original code generate a Random object in each getRandom call, I think it's resource waste, But I agree to use current system time will cause less secure. will change to use default constructor of the SecureRandom class.
There was a problem hiding this comment.
changed in 154167d
| else { | ||
| if (previous != null) { | ||
| _p2pMgr.closeSocket(node.getChannel(), | ||
| "outbound -> active, node " + previous.getIdShort() + " exits"); |
There was a problem hiding this comment.
Minor typo: it should read "exists" instead of "exits"
| activeNodes.clear(); | ||
| } | ||
|
|
||
| } catch (Exception e) { | ||
|
|
||
| e.printStackTrace(); |
There was a problem hiding this comment.
May be log the exception?
|
|
||
| private static final int TOTAL_LANE = Math | ||
| .min(Runtime.getRuntime().availableProcessors() << 1, 32); | ||
| private static final int threadQlimit = 20000; |
There was a problem hiding this comment.
Minor: threadQlimit could be all caps since its a constant
| @@ -72,8 +92,8 @@ public void run() { | |||
| } | |||
|
|
|||
| // if not belong to current lane, put it back. | |||
| int targetLane = hash2Lane(mo.getNodeId()); | |||
| if (targetLane != lane) { | |||
| long t1 = System.nanoTime(); | |||
There was a problem hiding this comment.
I don't see t1 being used, can be removed
There was a problem hiding this comment.
yep, it has been removed in the other PR #518
Notice
It is not allowed to submit your PR to the master branch directly, please submit your PR to the master-pre-merge branch.
Description
Please include a brief summary of the change that this pull request proposes. Include any relevant motivation and context. List any dependencies required for this change.
Also, the PR #518 including this PR, therefore, some of the comments will be revised in the #518.
Type of change
Insert x into the following checkboxes to confirm (eg. [x]):
Testing
Please describe the tests you used to validate this pull request. Provide any relevant details for test configurations as well as any instructions to reproduce these results.
Verification
Insert x into the following checkboxes to confirm (eg. [x]):