Skip to content

Add druid.indexer.server.maxChatRequests for QoS; deprecate separate ports.#2503

Merged
fjy merged 1 commit intoapache:masterfrom
gianm:jetty-qos
Feb 23, 2016
Merged

Add druid.indexer.server.maxChatRequests for QoS; deprecate separate ports.#2503
fjy merged 1 commit intoapache:masterfrom
gianm:jetty-qos

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Feb 19, 2016

  • Add druid.indexer.server.maxChatRequests, which sets up a QoSFilter on the main Jetty server.
  • Deprecate druid.indexer.runner.separateIngestionEndpoint
  • Deprecate druid.indexer.server.chathandler.*

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Feb 19, 2016

see discussion on #2419

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Feb 19, 2016

@gianm

Running io.druid.server.initialization.JettyQosTest
Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 53.158 sec <<< FAILURE! - in io.druid.server.initialization.JettyQosTest
testQoS(io.druid.server.initialization.JettyQosTest) Time elapsed: 52.729 sec <<< FAILURE!
java.lang.AssertionError: null
at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertTrue(Assert.java:52)
at io.druid.server.initialization.JettyQosTest.testQoS(JettyQosTest.java:191)

…ports.

- Add druid.indexer.server.maxChatRequests, which sets up a QoSFilter on the main Jetty server.
- Deprecate druid.indexer.runner.separateIngestionEndpoint
- Deprecate druid.indexer.server.chathandler.*
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Feb 19, 2016

@nishantmonu51 can you take a look?

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Feb 19, 2016

@fjy fixed the test

slowElapsed.addAndGet(System.currentTimeMillis() - startTime);
}
catch (InterruptedException e) {
// BE COOL
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Feb 19, 2016

👍

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Feb 19, 2016

would like feedback from at least a couple of @nishantmonu51 @drcrallen @guobingkun @pjain1 or @cheddar on this one, as it is a follow up to https://groups.google.com/forum/#!msg/druid-development/hfCggehFl_I/INfjEyxbBQAJ and deprecates the feature introduced in #1929.

@nishantmonu51
Copy link
Copy Markdown
Member

looks good to me, 👍 from my side, still keeping it open for others to review it.

@drcrallen
Copy link
Copy Markdown
Contributor

Suggest leaving open and bringing up in the dev sync to see if anyone wants to comment (also stating why this PR is needed). Then merge if no comment.

@cheddar
Copy link
Copy Markdown
Contributor

cheddar commented Feb 22, 2016

I didn't look too closely, but the goal of #1929 was to provide for clear separation of ingestion and query resources. As long as this maintains that (which it looks like it does), I'm on board.

@pjain1
Copy link
Copy Markdown
Member

pjain1 commented Feb 22, 2016

👍

fjy added a commit that referenced this pull request Feb 23, 2016
Add druid.indexer.server.maxChatRequests for QoS; deprecate separate ports.
@fjy fjy merged commit 93540c0 into apache:master Feb 23, 2016
Jerseys.addResource(binder, ChatHandlerResource.class);

if (properties.containsKey(MAX_CHAT_REQUESTS_PROPERTY)) {
final int maxRequests = Integer.parseInt(MAX_CHAT_REQUESTS_PROPERTY);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't believe I missed this... I'm pretty sure this is NOT what you wanted to do. Also this needs a parse catcher

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really good point

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it need a parse catcher? I think it's ok for this to blow up and let Guice report the error if you pass something that isn't a number

@gianm gianm deleted the jetty-qos branch July 18, 2018 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants