james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Harmeet Bedi" <harm...@kodemuse.com>
Subject Re: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
Date Mon, 05 Aug 2002 22:42:37 GMT
I was pointing out that the entire smtp id code is unnecessary. Removing
unnecessary code may be a good optimization.
smtpID variable and SMTP_ID state could be removed, so there would be no
need to optimize or look at it.

Harmeet


----- Original Message -----
From: "Peter M. Goldstein" <peter_m_goldstein@yahoo.com>
To: "'James Developers List'" <james-dev@jakarta.apache.org>
Sent: Monday, August 05, 2002 11:26 AM
Subject: RE: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt


>
> Harmeet,
>
> I just don't get your objection.
>
> SMTP ID does one thing and one thing only - allows you to identify from
> the external SMTP exchange which connection/thread it was associated
> with.  It's not a tremendously useful feature I grant you, but in the
> case of an extreme fatal error (one that resulted in a thread dump) it
> may very well allow you to connection the external SMTP connection with
> the single thread among potentially hundreds that was connecting to a
> particular SMTP exchange.  Presumably it was added to the code base for
> precisely that reason.  I'm reluctant to change an exposed, functioning
> piece of code from the software without any sort of compelling reason.
>
> The cost of this feature is negligible.  The Random is now shared
> between handlers, as opposed to an instantiation per thread (my change),
> the cost of a nextInt() invocation is minimal, and it's not a
> particularly large piece of state to track.  It's commented, so there's
> no code clarity issue.  So what's the problem?
>
> --Peter
>
> > -----Original Message-----
> > From: Harmeet Bedi [mailto:harmeet@kodemuse.com]
> > Sent: Monday, August 05, 2002 1:59 PM
> > To: James Developers List
> > Subject: Re: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
> >
> > Here is what I see in the latest code for variable smtpID.
> >
> > Declaration:
> >     private String smtpID;
> >
> > in <handleConnection>
> >             smtpID = Math.abs(random.nextInt() % 1024) + "";
> >
> > in <resetState>
> >         state.put(SMTP_ID, smtpID);
> >
> >
> > for state 'SMTP_ID'
> >
> > in <resetState>
> >         state.put(SMTP_ID, smtpID);
> >
> > in <doDATA>:
> >                 headers.addHeaderLine(
> >                     "Received: from "
> >                         + state.get(REMOTE_NAME)
> >                         + " (["
> >                         + state.get(REMOTE_IP)
> >                         + "])");
> >                 String temp =
> >                     "          by "
> >                         + this.helloName
> >                         + " ("
> >                         + softwaretype
> >                         + ") with SMTP ID "
> >                         + state.get(SMTP_ID);
> > .....
> >                     headers.addHeaderLine(temp);
> >
> >
> >
> > If SMTP_ID only contains random information, it really
> > contains no meaningful information. What is the gain of having it in
> the
> > mail header body ?
> >
> > How can a random integer value identify a particular thread/connection
> > ?
> >
> > The name 'SMTP_ID' lead me to believe this was the smtp server
> > identifier and that may make sense. There may be a need to identify
> > from the message header the routing mail server. I think helo name
> > parameter could be used to do this too, as you suggested. It may be a
> > good idea to rename or remove SMTP_ID and smtpID in that case.
> >
> > Harmeet
> > ----- Original Message -----
> > From: "Peter M. Goldstein" <peter_m_goldstein@yahoo.com>
> > To: "'James Developers List'" <james-dev@jakarta.apache.org>
> > Sent: Monday, August 05, 2002 10:37 AM
> > Subject: RE: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
> >
> >
> > >
> > > Harmeet,
> > >
> > > As written, both before and after the patch, this simply isn't what
> the
> > > SMTP ID does.  The SMTP ID is written to help identify the
> particular
> > > thread/connection associated with the handler.  That's all.  That's
> why
> > > it's randomly drawn from a pool of ids [0,1024).  It is in no way
> > > intended to identify a particular mail server.
> > >
> > > The helloName parameter can be used to address the issue you
> suggest.
> > > Each server in your cluster can be given a distinguished helloName.
> > >
> > > --Peter
> > >
> > > > -----Original Message-----
> > > > From: Harmeet Bedi [mailto:harmeet@kodemuse.com]
> > > > Sent: Monday, August 05, 2002 1:29 PM
> > > > To: James Developers List
> > > > Subject: Re: [PATCH] SMTPHandler.java v.2 - SMTP ID,
> Random.nextInt
> > > >
> > > > From: "Peter M. Goldstein"
> > > >
> > > > > In addition, the synchronized wrapper around the call to
> > > > > random.nextInt() has also been removed.  After consultation with
> the
> > > > > documentation I realized that Random is completely thread-safe
> by
> > > design
> > > > > and hence no external synchronization is required.
> > > >
> > > > This is called in the context of creating and SMTP ID. I think
> SMTP ID
> > > is
> > > > needed. It may be a nice convention, but one that is not uniformly
> > > > followed.
> > > > Software version may be sufficient. In any case random smtp id,
> > > doesn't
> > > > provide new information. If we do want to put out an smtp id, it
> may
> > > be
> > > > best
> > > > to have it as a configurable setting. This may be used to identify
> on
> > > mail
> > > > server out of a cluster of mail servers.
> > > >
> > > > Harmeet
> > > > ----- Original Message -----
> > > > From: "Peter M. Goldstein" <peter_m_goldstein@yahoo.com>
> > > > To: "'James Developers List'" <james-dev@jakarta.apache.org>
> > > > Sent: Sunday, August 04, 2002 10:26 AM
> > > > Subject: [PATCH] SMTPHandler.java v.2
> > > >
> > > >
> > > > >
> > > > > All,
> > > > >
> > > > > Attached is a patch to the SMTPHandler.java code that rolls in
> the
> > > > > String.equalsIgnoreCase change that Noel suggested.
> > > > >
> > > > > In addition, the synchronized wrapper around the call to
> > > > > random.nextInt() has also been removed.  After consultation with
> the
> > > > > documentation I realized that Random is completely thread-safe
> by
> > > design
> > > > > and hence no external synchronization is required.
> > > > >
> > > > > All other previously made changes were left intact.
> > > > >
> > > > > Attached is the source and diff.  As always, comments,
> criticisms,
> > > and
> > > > > questions are welcome.
> > > > >
> > > > > --Peter
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Noel J. Bergman [mailto:noel@devtech.com]
> > > > > > Sent: Saturday, August 03, 2002 8:59 PM
> > > > > > To: James-Dev Mailing List
> > > > > > Subject: String.equalsIgnoreCase Considered Evil :-)
> > > > > >
> > > > > > There are many places in James where we have lengthy sets of
> > > > > comparisons
> > > > > > of
> > > > > > an unknown string value to a known string value.  These are
> many
> > > of
> > > > > these
> > > > > > of
> > > > > > the form:
> > > > > >
> > > > > > string.equalsIgnoreCase(<literal>)
> > > > > >
> > > > > > This use is terribly inefficient.  Each and every call
> iterates
> > > > > through
> > > > > > both
> > > > > > the string and the literal, uppercasing both as it compares
> them.
> > > > > >
> > > > > > Please, as you are working on code (Peter will apply these to
> > > > > POP3Handler
> > > > > > and SMTPHandler), if you see this pattern, please change it
> to:
> > > > > >
> > > > > > string = string.to[Upper|Lower]Case();  // chose depending
> upon
> > > > > your
> > > > > > literals
> > > > > >
> > > > > > string.equals(<literal>)
> > > > > >
> > > > > > For example, in SMTPHandler and POP3Handler:
> > > > > >
> > > > > >       String command = commandRaw.trim();
> > > > > >  becomes
> > > > > > String command = commandRaw.trim().toUpperCase();
> > > > > >
> > > > > > and the test for "USER" (for example) becomes:
> > > > > >
> > > > > >         if (command.equals("USER")) ...
> > > > > >
> > > > > > Actually, I believe that we should add a command-map model to
> the
> > > > > > handlers,
> > > > > > but that's a seperate issue for a separate thread.  The change
> > > > > proposed in
> > > > > > this e-mail is simple.
> > > > > >
> > > > > > --- Noel
> > > > > >
> > > > > > P.S.  Brownie points for whomever recognizes the origin of the
> > > subject
> > > > > > header
> > > > > >
> > > > > >
> > > > > > --
> > > > > > To unsubscribe, e-mail:   <mailto:james-dev-
> > > > > > unsubscribe@jakarta.apache.org>
> > > > > > For additional commands, e-mail: <mailto:james-dev-
> > > > > > help@jakarta.apache.org>
> > > > >
> > > >
> > > >
> > > >
> > >
> ------------------------------------------------------------------------
> > > --
> > > > --
> > > > ----
> > > >
> > > >
> > > > > --
> > > > > To unsubscribe, e-mail:
> > > > <mailto:james-dev-unsubscribe@jakarta.apache.org>
> > > > > For additional commands, e-mail:
> > > > <mailto:james-dev-help@jakarta.apache.org>
> > > >
> > > >
> > > > --
> > > > To unsubscribe, e-mail:   <mailto:james-dev-
> > > > unsubscribe@jakarta.apache.org>
> > > > For additional commands, e-mail: <mailto:james-dev-
> > > > help@jakarta.apache.org>
> > >
> > >
> > >
> > > --
> > > To unsubscribe, e-mail:
> > <mailto:james-dev-unsubscribe@jakarta.apache.org>
> > > For additional commands, e-mail:
> > <mailto:james-dev-help@jakarta.apache.org>
> >
> >
> > --
> > To unsubscribe, e-mail:   <mailto:james-dev-
> > unsubscribe@jakarta.apache.org>
> > For additional commands, e-mail: <mailto:james-dev-
> > help@jakarta.apache.org>
>
>
>
> --
> To unsubscribe, e-mail:
<mailto:james-dev-unsubscribe@jakarta.apache.org>
> For additional commands, e-mail:
<mailto:james-dev-help@jakarta.apache.org>


--
To unsubscribe, e-mail:   <mailto:james-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:james-dev-help@jakarta.apache.org>


Mime
View raw message