Skip to content

fix missing property in JsonTypeInfo of SegmentWriteOutMediumFactory - #6656

Merged
jihoonson merged 1 commit into
apache:masterfrom
QiuMM:fix_segment
Nov 27, 2018
Merged

jihoonson merged 1 commit into
apache:masterfrom
QiuMM:fix_segment

Conversation

@QiuMM

@QiuMM QiuMM commented Nov 23, 2018 •

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

@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

fjy commented Nov 26, 2018

Copy link
Copy Markdown
Contributor

@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

QiuMM commented Nov 26, 2018

Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown
Contributor

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
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