Use Combined*SequenceFile for VKVS, WritableSequenceFileScheme, SequenceFileScheme #1647

dieu · 2017-02-28T00:40:54Z

That give us the flexibility to combine small files in fewer mappers.

johnynek · 2017-02-28T00:50:23Z

how was this tested and can you add any comments about the motivation or test/benchmark results?

dieu · 2017-02-28T02:17:00Z

@johnynek motivation is simple, in many cases, VKVS produce small files and these changes give us the flexibility to have ability combine input of VKVS in fewer mappers.

Performance impact should mitigate by tuning settings of maxSplitSize.

and scalding has tests for VKVS (VersionedKeyValSourceTest).

pankajroark · 2017-02-28T06:49:16Z

May be worth running our Tsar end-to-end tests with this change to verify.

gerashegalov · 2017-02-28T07:26:04Z

scalding-commons/src/main/java/com/twitter/scalding/commons/scheme/KeyValueByteScheme.java

@@ -24,6 +26,11 @@ public KeyValueByteScheme(Fields fields) {
    super(fields, BytesWritable.class, BytesWritable.class);
  }

+  @Override
+  public void sourceConfInit(FlowProcess<JobConf> flowProcess, Tap<JobConf, RecordReader, OutputCollector> tap, JobConf conf) {
+    conf.setInputFormat(CombineSequenceFileInputFormat.class);


I don't think it's a good idea to hardcode Hadoop's CombineSequenceFileInputFormat especially since we normally use the one from ElephnantBird. It's also disabled by default which will make it a backwards-compatible change.

The simplest way to hook it up in VersionedKeyValSource

import com.twitter.elephantbird.cascading2.scheme.CombinedSequenceFile ... def hdfsScheme = HadoopSchemeInstance(new CombinedSequenceFile(fields).asInstanceOf[Scheme[_, _, _, _, _]])

I did little refactoring to keep backward capability.

My one-line change suggestion for VersionedKeyValSource is not sufficient?

I believe that it should be opt-in and affect not only VKVS.

And also, it better to moves to hadoop implementation instead elephantbird.

EB is opt-in, you have to set elephantbird.use.combine.input.format=true
How do you measure "better" for hadoop's impl. The code change is simpler in the EB case.

You can make input format configurable.

Hadoop has better community support, EB used only inside Twitter.

@isnotinvain / @johnynek what do you think?

johnynek · 2017-03-01T03:42:35Z

now this is an even bigger change, where we need even more evidence it is safe. Unfortunately, sources are things that are hard to test well because you really need to interact with Hadoop. He have some hadoop tests, but we do not have performance or memory tests.

I agree that using standard hadoop is preferable if possible. That said, we already have an elephant-bird dependency.

@gerashegalov what is it about the elephantbird implementation that is better here? It is a shame if we should assume a custom approach because it is indeed hard for people to keep it straight.

gerashegalov · 2017-03-01T06:44:08Z

@johnynek we widely use EB combiner with LzoThrift (and even Parquet via Multiformat sources), whereas I am not aware of significant usage of this Hadoop class. We use EB CombinedSequenceFile specifically, specifically on thousands jobs for intermediate data daily. I also pointed out the simplicity of such a change with the EB. Other than that I don't have any data to compare.

However, as stated before, we can make it configurable to give users a choice.

dieu · 2017-03-01T18:50:04Z

@johnynek

I think that now it safer changes because at now it's opt-in and if a user doesn't turn on the flag then nothing will change for him.
Performance/Memory will be affected only when user will turn on this feature if he does it, he understands how to tune it and why he need it.
I will run our internal e2e tests if will agree that this approach is good to merge.
I understand that we already have EB as the dependency, but we need understand when and in which case we want to use Hadoop API.

dieu · 2017-03-09T21:23:56Z

@gerashegalov I changed approach to use EB API, can you look at now?

gerashegalov

👍 LGTM

gerashegalov · 2017-03-09T22:14:11Z

build.sbt

@@ -329,6 +329,8 @@ lazy val scaldingCore = module("core").settings(
    "com.twitter" %% "bijection-macros" % bijectionVersion,
    "com.twitter" %% "chill" % chillVersion,
    "com.twitter" %% "chill-algebird" % chillVersion,
+    "com.twitter.elephantbird" % "elephant-bird-cascading2" % elephantbirdVersion,


@johnynek what's your take on our favorite subject of transitive dependencies? If they are allowed we can remove eb entries from scaldingCommons.

so, strictly speaking, I thought elephant-bird had GPL'd dependencies (lzo). That was actually one of the original reasons for not putting this in the core.

Do we need both of these dependencies here?

I start to wonder if there is a single consumer of elephant-bird-cascading2 other than scalding. Looks like the answer is no:
https://mvnrepository.com/artifact/com.twitter.elephantbird/elephant-bird-cascading2/usages

Maybe we should move that code into this repo so we don't have the cross repo publish issue?

We could move it into a separate module, like we did with maple and just have the java code there. As far as elephant-bird-core, that is used elsewhere. I wonder if the gpl lzo code can be is a separate module in elephant-bird, then we could have scalding-lzo that depends on that and split scalding-commons into core and lzo?

Each new dependency is a possible pain point for others... So, I think we should add dependencies very carefully (and usually strive to remove them, not add them).

@johnynek got it, let's keep elephantbird isolated in commons for now and use combined for VKVS and have separated CombinedSchemas for sequence files as helpers.

dieu · 2017-03-24T19:19:11Z

@gerashegalov / @johnynek please review again :)

johnynek · 2017-03-24T20:58:43Z

...-commons/src/main/scala/com/twitter/scalding/commons/scheme/CombinedSequenceFileScheme.scala

+trait CombinedWritableSequenceFileScheme extends WritableSequenceFileScheme {
+  // TODO Cascading doesn't support local mode yet
+  override def hdfsScheme =
+    HadoopSchemeInstance(new CombinedWritableSequenceFile(fields, keyType, valueType).asInstanceOf[cascading.scheme.Scheme[_, _, _, _, _]])


why the fully qualified name here but not on line 10?

done, thanks

isnotinvain · 2017-03-25T00:17:31Z

Side note, one reason to use EB's combine input format and not hadoop's is I noticed that the hadoop one doesn't properly report mapper progress for some reason (mappers go from 0% to 100% and nothing in between). Maybe it's not that commonly used.

johnynek · 2017-03-28T18:32:42Z

👍

…SequenceFileScheme

dieu requested a review from isnotinvain February 28, 2017 00:41

gerashegalov suggested changes Feb 28, 2017

View reviewed changes

dieu force-pushed the apanasenko/vkvs_to_combine_input_seq_file branch from 87f23ed to e224b32 Compare February 28, 2017 19:02

dieu force-pushed the apanasenko/vkvs_to_combine_input_seq_file branch from e224b32 to 3d881e3 Compare March 9, 2017 02:03

dieu changed the title ~~Move KeyValueByteScheme (VersionedKeyValSource) to use CombineSequenceFileInputFormat~~ Use Combined*SequenceFile for VKVS, WritableSequenceFileScheme, SequenceFileScheme, CodecSource Mar 9, 2017

dieu requested a review from piyushnarang March 9, 2017 02:09

gerashegalov approved these changes Mar 9, 2017

View reviewed changes

dieu force-pushed the apanasenko/vkvs_to_combine_input_seq_file branch from 4cb54dd to 40dbad9 Compare March 22, 2017 01:03

dieu changed the title ~~Use Combined*SequenceFile for VKVS, WritableSequenceFileScheme, SequenceFileScheme, CodecSource~~ Use Combined*SequenceFile for VKVS, WritableSequenceFileScheme, SequenceFileScheme Mar 22, 2017

dieu force-pushed the apanasenko/vkvs_to_combine_input_seq_file branch from 40dbad9 to 38a1cd9 Compare March 22, 2017 21:05

piyushnarang mentioned this pull request Mar 23, 2017

Request for Scalding release 0.17.0 #1641

Merged

dieu force-pushed the apanasenko/vkvs_to_combine_input_seq_file branch from 38a1cd9 to c416872 Compare March 24, 2017 19:18

johnynek reviewed Mar 24, 2017

View reviewed changes

dieu force-pushed the apanasenko/vkvs_to_combine_input_seq_file branch from c416872 to b1927ce Compare March 24, 2017 21:09

Use CombinedWritableSequenceFile for VKVS and make separate Combined*…

d7c2708

…SequenceFileScheme

dieu force-pushed the apanasenko/vkvs_to_combine_input_seq_file branch from b1927ce to d7c2708 Compare March 28, 2017 18:37

dieu merged commit 115fd1a into twitter:develop Mar 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Combined*SequenceFile for VKVS, WritableSequenceFileScheme, SequenceFileScheme #1647

Use Combined*SequenceFile for VKVS, WritableSequenceFileScheme, SequenceFileScheme #1647

dieu commented Feb 28, 2017 •

edited

Loading

johnynek commented Feb 28, 2017

dieu commented Feb 28, 2017 •

edited

Loading

pankajroark commented Feb 28, 2017

gerashegalov Feb 28, 2017

dieu Feb 28, 2017

gerashegalov Feb 28, 2017

dieu Feb 28, 2017

gerashegalov Feb 28, 2017

dieu Feb 28, 2017

johnynek commented Mar 1, 2017

gerashegalov commented Mar 1, 2017 •

edited

Loading

dieu commented Mar 1, 2017

dieu commented Mar 9, 2017

gerashegalov left a comment

gerashegalov Mar 9, 2017

johnynek Mar 10, 2017

dieu Mar 22, 2017

dieu commented Mar 24, 2017

johnynek Mar 24, 2017

dieu Mar 24, 2017

isnotinvain commented Mar 25, 2017

johnynek commented Mar 28, 2017

Use Combined*SequenceFile for VKVS, WritableSequenceFileScheme, SequenceFileScheme #1647

Use Combined*SequenceFile for VKVS, WritableSequenceFileScheme, SequenceFileScheme #1647

Conversation

dieu commented Feb 28, 2017 • edited Loading

johnynek commented Feb 28, 2017

dieu commented Feb 28, 2017 • edited Loading

pankajroark commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Mar 1, 2017

gerashegalov commented Mar 1, 2017 • edited Loading

dieu commented Mar 1, 2017

dieu commented Mar 9, 2017

gerashegalov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dieu commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isnotinvain commented Mar 25, 2017

johnynek commented Mar 28, 2017

dieu commented Feb 28, 2017 •

edited

Loading

dieu commented Feb 28, 2017 •

edited

Loading

gerashegalov commented Mar 1, 2017 •

edited

Loading