Skip to content

Merge druid-api, druid-common and java-util modules into druid-common#5490

Closed
leventov wants to merge 1 commit intoapache:masterfrom
metamx:merge-druid-common
Closed

Merge druid-api, druid-common and java-util modules into druid-common#5490
leventov wants to merge 1 commit intoapache:masterfrom
metamx:merge-druid-common

Conversation

@leventov
Copy link
Copy Markdown
Member

I had to collapse druid-api, druid-common and java-util, to avoid cyclic dependencies. It was a part of #4312 plan.

First part of #5488.

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 we fix this TODO?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not going to do this in this PR

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.

okay, can we add more context to what the TODO means and link it to an issue?

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.

I think it makes sense, in this PR, to change nothing about the content and just collapse the modules.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@b-slim previous comment was a lengthy explanation why those config classes couldn't be merged at that time. Now they could. I didn't want to add explanations why it should be done, because previously it wasn't as well (it is considered obvious, I suppose.)

Anyway, in general we don't usually add TODOs and favor issues on github; I've created #5491.

@b-slim
Copy link
Copy Markdown
Contributor

b-slim commented Mar 14, 2018

looks good to me, but there are some merge conflicts.

@jihoonson
Copy link
Copy Markdown
Contributor

@leventov would you resolve conflicts? I think this PR should be merged as soon as possible to avoid further conflicts.

@jihoonson
Copy link
Copy Markdown
Contributor

This patch looks good to me. I'll finish my review if CI is fixed.

@leventov
Copy link
Copy Markdown
Member Author

Unfortunately I don't know how to fix the CI. That are strange problems that I cannot reproduce locally. Any ideas welcome.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Mar 23, 2018

The failure in LoggingRequestLoggerTest looks like a bug in the test. It has this code,

  @BeforeClass
  public static void setUpStatic() throws Exception
  {
    appender = OutputStreamAppender
        .newBuilder()
        .setName("test stream")
        .setTarget(baos)
        .setLayout(JsonLayout.createLayout(false, true, false, true, true, Charsets.UTF_8))
        .build();
    final Logger logger = (Logger)
        LogManager.getLogger(LoggingRequestLogger.class);
    appender.start();
    logger.addAppender(appender);
  }

  @After
  public void tearDown()
  {
    baos.reset();
  }

But this is wrong. baos is created in a static @BeforeClass setup method but is cleared in a per-test @After method. It leads to the "No content" error from Jackson. I'm honestly not sure how this has ever passed properly. Maybe some test configuration had previously been doing forks per test and now isn't?

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Mar 23, 2018

I'm not sure what causes the other, docker integration test failure.

@jihoonson
Copy link
Copy Markdown
Contributor

I can reproduce the IT test failure locally. I'm still not sure what's the problem but I could see every Druid servers work well.

@jihoonson
Copy link
Copy Markdown
Contributor

jihoonson commented Mar 28, 2018

@leventov ok. The unit test failures in LoggingRequestLoggerTest are because of the wrong log level. I don't know why, but the log level is set to ERROR and INFO level logs are not written.

The integration tests failure looks somehow related to service discovery. I can see these logs a lot in the overlord log. I checked mysql, zookeeper, and druid coordinator were running well.

2018-03-07T20:57:02,601 ERROR [BasicAuthenticatorCacheManager-Exec--0] io.druid.curator.discovery.ServerDiscoverySelector - No server instance found for [druid/coordinator]

BTW, you should be able to reproduce the test failures locally by deleting the maven local cache and installing druid.

@leventov leventov force-pushed the merge-druid-common branch from fad7561 to 0e1a3c1 Compare April 7, 2018 08:59
@leventov leventov force-pushed the merge-druid-common branch from 571725b to 7eec889 Compare April 7, 2018 12:42
@leventov leventov closed this Apr 7, 2018
@leventov leventov deleted the merge-druid-common branch April 7, 2018 15:50
@leventov
Copy link
Copy Markdown
Member Author

leventov commented Apr 7, 2018

I give up trying to do this

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.

4 participants