Smaller chunks would be nice, I appreciate that isn't always feasible. Code
review follows...
On Tuesday 18 December 2007 21:22, robert at freenetproject.org wrote:
> Author: robert
> Date: 2007-12-18 21:22:13 +0000 (Tue, 18 Dec 2007)
> New Revision: 16701
>
> Modified:
> trunk/freenet/src/freenet/node/CHKInsertSender.java
> Log:
> simplify, factor out Sender, receivedCompletionNotice was not being set
>
>
> Modified: trunk/freenet/src/freenet/node/CHKInsertSender.java
> ===================================================================
> --- trunk/freenet/src/freenet/node/CHKInsertSender.java 2007-12-18
> 20:16:10
UTC (rev 16700)
> +++ trunk/freenet/src/freenet/node/CHKInsertSender.java 2007-12-18
> 21:22:13
UTC (rev 16701)
> @@ -24,36 +24,7 @@
>
> public final class CHKInsertSender implements Runnable, AnyInsertSender,
ByteCounter {
>
> - private static class Sender implements Runnable {
> -
> - final AwaitingCompletion completion;
> - final BlockTransmitter bt;
> - final Executor executor;
> -
> - public Sender(AwaitingCompletion ac, Executor executor) {
> - this.bt = ac.bt;
> - this.completion = ac;
> - this.executor = executor;
> - }
> -
> - public void run() {
> - freenet.support.Logger.OSThread.logPID(this);
> - try {
> - bt.send(executor);
> - if(bt.failedDueToOverload()) {
> - completion.completedTransfer(false);
> - } else {
> - completion.completedTransfer(true);
> - }
> - } catch (Throwable t) {
> - completion.completedTransfer(false);
> - Logger.error(this, "Caught "+t, t);
> - }
> - }
> - }
> -
> - private class AwaitingCompletion {
> -
> + private class AwaitingCompletion implements Runnable {
> /** Node we are waiting for response from */
> final PeerNode pn;
> /** We may be sending data to that node */
> @@ -79,34 +50,32 @@
> }
>
> void start() {
> - Sender s = new Sender(this, node.executor);
> - node.executor.execute(s, "Sender for "+uid+" to
> "+pn.getPeer());
> + node.executor.execute(this,
> "CHKInsert-AwaitingCompletion for "+uid+"
to "+pn.getPeer());
> }
>
> - void completed(boolean timeout, boolean success) {
> - synchronized(this) {
> - if(timeout)
> - completionTimedOut = true;
> - else
> - completionSucceeded = success;
> - receivedCompletionNotice = true;
> - notifyAll();
> - }
> - synchronized(nodesWaitingForCompletion) {
> - nodesWaitingForCompletion.notifyAll();
> - }
> - if(!success) {
> - synchronized(CHKInsertSender.this) {
> - transferTimedOut = true;
> - CHKInsertSender.this.notifyAll();
> + public void run() {
> + freenet.support.Logger.OSThread.logPID(this);
> + try {
> + bt.send(node.executor);
> + if(bt.failedDueToOverload()) {
> + this.completed(false, false);
> + } else {
> + this.completed(false, true);
> }
> + } catch (Throwable t) {
> + this.completed(false, false);
> + Logger.error(this, "Caught "+t, t);
> }
> }
Okay so here you have moved Sender.run() into AwaitingCompletion, and deleted
Sender as it's now redundant. Nice simplification but ideally it should be
separate.
>
> - void completedTransfer(boolean success) {
> + void completed(boolean timeout, boolean success) {
> + if (logMINOR) Logger.minor(this,
> "CHKInsert-AwaitingCompletion complete
(timeout="+timeout+", success="+success);
> + if (success && timeout)
> + throw new IllegalArgumentException("how can a
> request successfully
timeout?");
This is just logging.
> synchronized(this) {
> - transferSucceeded = success;
> - completedTransfer = true;
> + completionTimedOut = timeout;
> + completionSucceeded = success;
> + receivedCompletionNotice = true;
> notifyAll();
> }
Okay, there's something interesting happening here...
receivedCompletionNotice means we have not only finished the transfer but also
received a message indicating that downstream has also finished transferring.
AFAICS you are setting it here to indicate we have finished the transfer ???
There is a reason for this behaviour: We need a realistic round-trip time for
the AIMD that controls the number of inserts that we start. If we only take
into account our own transfer time that isn't very realistic, as downstream
can take much longer to complete sometimes. Obviously AIMD will often run
many requests at once, but it needs a correct RTT.
> synchronized(nodesWaitingForCompletion) {
> @@ -444,7 +413,6 @@
> if (msg.getSpec() == DMT.FNPRouteNotFound) {
> if(logMINOR) Logger.minor(this,
> "Rejected: RNF");
> short newHtl = msg.getShort(DMT.HTL);
> - Logger.error(this, "CHKInsert-RNF:
> htl="+htl+", msg.htl="+newHtl);
> synchronized (this) {
> if (htl > newHtl)
> htl = newHtl;
>
> @@ -697,8 +665,9 @@
> timeout = (int)Math.min(Integer.MAX_VALUE,
> (transfersCompletedTime +
TRANSFER_COMPLETION_ACK_TIMEOUT) - now);
> if(timeout <= 0) {
> synchronized(CHKInsertSender.this) {
> - if(logMINOR) Logger.minor(this,
> "Timed out waiting for transfers to
complete on "+uid);
> + Logger.error(this, "Timed out
> waiting for transfers to complete
on "+uid);
> transferTimedOut = true;
> +
> //CHKInsertSender.this.notifyAll();
Why?
> }
> return;
> }
> @@ -743,9 +712,9 @@
> }
> if(m == null) {
> Logger.error(this, "Timed out
> waiting for a final ack from any
nodes.");
> - //Would looping again help? We
> will either:
> - // (1) time out again (and be
> right back here if there is more time
left), or
> - // (2) notice that the nodes we
> are waiting on are down and exit
immediately.
> + //Would looping again help? We
> could either:
> + // (1) loop and notice that
> there is no more time left (handling the
timeout), or
> + // (2) notice that the nodes we
> are waiting on are down and possibly
exit gracefully (not implemented).
> continue;
> } else {
> // Process message
Comments fix...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL:
<https://emu.freenetproject.org/pipermail/devl/attachments/20071218/455f1972/attachment.pgp>
