james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Peter M. Goldstein" <peter_m_goldst...@yahoo.com>
Subject RE: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
Date Mon, 05 Aug 2002 18:26:10 GMT

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>


Mime
View raw message