[
https://issues.apache.org/jira/browse/FLUME-721?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13090851#comment-13090851
]
[email protected] commented on FLUME-721:
-----------------------------------------------------
bq. On 2011-08-19 23:27:24, Eric Sammer wrote:
bq. > We now expose Jetty dependencies (and enforce them) on consumers of
InternalHttpServer. We've further deviated from normal war deployment
complicating things for developers. Once again, we need to modify the core of
Flume to add / remove / update its web applications which completely defeats
the purpose of the original refactoring. The correct way to solve the missing
log view and stacks servlet errors was to just create two wars (or even one)
with the applications and drop it into the webapps directory; none of that
works anymore. I disagree with the approach entirely but I'm positive it works
as advertised. If we were going to just rewrite this back to StatusHttpServer,
we should have just reverted the InternalHttpServer patch (because that's what
this and the last change effectively do).
I understand that you are saying that there is ideal approach that I'm not
using. I'm also very happy to take a patch that updates the implementation to
use "stardard" war deployment. Admittedly I'm not an expert on standard war
deployments (arvind knows more about this). What is vital to me is that we the
ability to provide the basic mechanism to debug a user's running system. To me
this is a vital requirement that trumps idealism.
The ideal version would also be user-friendly, developer-friendly, and
"standard". While you claim that 0.9.4 version is developer friendly, as a
developer I tend to disagree -- we've lost the ability to debug jsp based pages
in a IDE (if possible please update the old instructions on how to do this),
lost the ability to debug jsp's live (again instructions please), lost unit
tests, lost stack dumps, and lost the autoincrementing port feature that
enables one to test on a single machine.
The main improvement I see the 0.9.4 version and this version of the
InternalHttpServer is a better decoupling. The 0.9.4 version still depended on
FlumeConfiguration and this version no longer depends on it to look for
specific wars in a particular directory (it provides statics that instantiators
could call/use).
bq. On 2011-08-19 23:27:24, Eric Sammer wrote:
bq. > flume-core/src/main/java/com/cloudera/flume/master/FlumeMaster.java,
line 273
bq. > <https://reviews.apache.org/r/1604/diff/1/?file=33961#file33961line273>
bq. >
bq. > A clean way to do this would be to configure a factory and have it
return configured instances of InternalHttpServer. What's missing is a DI
container to externalize the wiring of contexts -> http server -> flume node.
patches welcome (see rule of 3 above)
bq. On 2011-08-19 23:27:24, Eric Sammer wrote:
bq. > flume-core/src/main/java/com/cloudera/util/InternalHttpServer.java,
lines 164-174
bq. > <https://reviews.apache.org/r/1604/diff/1/?file=33963#file33963line164>
bq. >
bq. > By removing this we've effectively moved back out of the normal
deployment model. I don't understand why we can't adhere to conventions.
The first case is an unused code path that would potentially cause unexpected
behavior (if I put in the webapps dir as opposed to a war file, it would load
th master and node war which is not what is supposed to happen).
bq. On 2011-08-19 23:27:24, Eric Sammer wrote:
bq. > flume-core/src/main/java/com/cloudera/flume/agent/FlumeNode.java, line 37
bq. > <https://reviews.apache.org/r/1604/diff/1/?file=33960#file33960line37>
bq. >
bq. > The whole point of InternalHttpServer was to eliminate Jetty
references from FlumeNode.
My goal was to eliminate refrences of Flume from InternalHttpServer. I feel
that Flume needs to be able to inject flume specific servlets that will be
served.
bq. On 2011-08-19 23:27:24, Eric Sammer wrote:
bq. > flume-core/src/main/java/com/cloudera/flume/agent/FlumeNode.java, line
265
bq. > <https://reviews.apache.org/r/1604/diff/1/?file=33960#file33960line265>
bq. >
bq. > A nicer way to handle this would be with dependencies injection.
This just hard codes the binding here (rather than in InternalHttpServer) so it
doesn't buy us anything in terms of abstraction or control.
If you want to select a list of wars via some specification here (like jetty's
xml file --
http://docs.codehaus.org/display/JETTY/Jetty+Xml+Configuration+Syntax+Reference
) that is definitely possible. This seems like something reasonable to do
when we add https:// or the ability to disable certain servlets.
In this case, there are only two instances (FlumeNode FlumeMaster) so I hard
code. (I have a rule of 3 -- copy and paste up to 3x, afterwards refactor).
bq. On 2011-08-19 23:27:24, Eric Sammer wrote:
bq. > flume-core/src/main/java/com/cloudera/util/InternalHttpServer.java,
lines 236-237
bq. > <https://reviews.apache.org/r/1604/diff/1/?file=33963#file33963line236>
bq. >
bq. > Further exposure of Jetty dependency.
This is a limitation if we want the ability to auto-increment the port
selected. I tried reopening the after a bind attempt failed and the contexts
did not get setup properly.
- jmhsieh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1604/#review1577
-----------------------------------------------------------
On 2011-08-19 22:07:34, jmhsieh wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/1604/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-08-19 22:07:34)
bq.
bq.
bq. Review request for Flume, Arvind Prabhakar and Eric Sammer.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This was previously posted at https://review.cloudera.org/r/1894/ and
pushed because of timeout. The patch actually was later then reverted because
it had some problems with imports. Since the previous patch had timed-out as
opposed to being actually reviewed, I'm reposting for review.
bq.
bq.
bq. FLUME-721: Webapps 'autofindport' feature does not work
bq.
bq. This refactors the internal http server so that context are created by
a callback object.
bq.
bq.
bq. This addresses bug flume-721.
bq. https://issues.apache.org/jira/browse/flume-721
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. flume-core/src/main/java/com/cloudera/flume/agent/FlumeNode.java b8f2b67
bq. flume-core/src/main/java/com/cloudera/flume/master/FlumeMaster.java
5c99c43
bq. flume-core/src/main/java/com/cloudera/util/HttpServerTestUtils.java
PRE-CREATION
bq. flume-core/src/main/java/com/cloudera/util/InternalHttpServer.java
1f41460
bq. flume-core/src/main/java/com/cloudera/util/StatusHttpServer.java 58c0ece
bq. flume-core/src/test/java/com/cloudera/flume/agent/TestNodeJersey.java
2625a99
bq. flume-core/src/test/java/com/cloudera/flume/master/TestMasterJersey.java
146cd33
bq. flume-core/src/test/java/com/cloudera/util/InternalHttpServerTest.java
b6a4cdd
bq. flume-core/src/test/java/com/cloudera/util/TestStatusHttpServer.java
ac47ac5
bq. flume-node-web/src/test/java/com/cloudera/flume/agent/TestBootstrap.java
8d241dd
bq.
bq. Diff: https://reviews.apache.org/r/1604/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. ran tests, they passed.
bq.
bq.
bq. Thanks,
bq.
bq. jmhsieh
bq.
bq.
> Webapps 'autofindport' feature does not work
> --------------------------------------------
>
> Key: FLUME-721
> URL: https://issues.apache.org/jira/browse/FLUME-721
> Project: Flume
> Issue Type: Bug
> Affects Versions: v0.9.4
> Reporter: Jonathan Hsieh
> Assignee: Jonathan Hsieh
> Priority: Critical
> Fix For: v0.9.5
>
> Attachments:
> 0001-FLUME-721-Webapps-autofindport-feature-does-not-work.patch,
> 0001-FLUME-721-Webapps-autofindport-feature-does-not-work.patch
>
>
> This is a regression in the upgrade and changes to the webapps from 0.9.3 to
> 0.9.4. In previous versions the default was to enable an option that
> automatically increment http server port in order if the desired port was had
> something bound to it. This made it easier to run and test multiple physical
> nodes on a single machine.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
