Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix missing property in JsonTypeInfo of SegmentWriteOutMediumFactory #6656

Merged
merged 1 commit into from Nov 27, 2018

Conversation

@QiuMM
Copy link
Member

commented Nov 23, 2018

Currently, we need to do like below when specify the SegmentWriteOutMediumFactory, which is terrible.

druid.peon.defaultSegmentWriteOutMediumFactory.@type: offHeapMemory

Besides, this PR also fixes the docs about SegmentWriteOutMediumFactory.

@leventov

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

@QiuMM please don't specify milestones for any non-bug PRs. I know that a lot of people do this, but it's a bad practice. See https://groups.google.com/d/msg/druid-development/QPZUIzLtZ2I/eZyw8BoVCgAJ.

@fjy

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

@leventov strongly disagree. I think where possible we should try to assign milestones to PRs we want to get in and aim to have the PR reviewed and merged before then. If the PR needs to be pushed back to a future release we can always do that.

@QiuMM

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2018

@leventov although this PR did not fix bug in the code, it fixed the docs. There is no effect when specify value for druid.peon.defaultSegmentWriteOutMediumFactory unless set value for druid.peon.defaultSegmentWriteOutMediumFactory.@type, but people do not know it. So this PR fixed this thing and because @type looks strange then I modified it. Since this PR is important(I think), I added this to the 0.13.1 milestone.

@jihoonson jihoonson self-assigned this Nov 27, 2018
@jihoonson

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

LGTM. I'm merging this PR.

Regarding milestone, @leventov started a thread on our dev mailing list (http://mail-archives.apache.org/mod_mbox/druid-dev/201811.mbox/%3CCAAMLo=Y_d1tXtgWpt_E7uD7zwh6=rDop-B_3O73HnQwBMRo2TQ@mail.gmail.com%3E), and so we can continue discussion there.

@jihoonson jihoonson merged commit 849ba86 into apache:master Nov 27, 2018
2 checks passed
2 checks passed
Inspections: pull requests (Druid) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.