Skip to content

[BEAM-2166] Split Coder's encode/decode methods into two methods depending on context.#2871

Closed
robertwb wants to merge 3 commits intoapache:masterfrom
robertwb:nested-coders
Closed

[BEAM-2166] Split Coder's encode/decode methods into two methods depending on context.#2871
robertwb wants to merge 3 commits intoapache:masterfrom
robertwb:nested-coders

Conversation

@robertwb
Copy link
Contributor

@robertwb robertwb commented May 3, 2017

…text.

This allows the outer context to be marked deprecated. A follow-up PR will
remove the old method once all consumers have been updated.

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify.
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

…text.

This allows the outer context to be marked deprecated.  A follow-up PR will
remove the old method once all consumers have been updated.
@robertwb
Copy link
Contributor Author

robertwb commented May 3, 2017

R: @lukecwik

Copy link
Member

@lukecwik lukecwik left a comment

Choose a reason for hiding this comment

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

return builder.toString();
}

public void encode(T value, OutputStream outStream)
Copy link
Member

@lukecwik lukecwik May 3, 2017

Choose a reason for hiding this comment

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

Add default implementations of:

  • encode(T, OutputStream, Context) that calls the appropriate encode(T, OutputStream)/encodeOuter(T, OutputStream) methods.
  • decode(InputStream, Context) that calls the appropriate decode(InputStream)/decodeOuter(InputStream) methods.

This way we can remove the base method as soon as everyone has migrated to implementing the non context versions.

Also, we should have:

  • encodeOuter(T, OutputStream) call encode(T, OutputStream)
  • decodeOuter(InputStream) call decode(InputStream)

This way only Coders where outer context is important will need to implement it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented the base encode/decode methods (though this opens us up to infinite recursion and no abstract methods, it should be short term). However, it's unsafe to unconditionally delegate the Outer ones to nested until everything is migrated.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 70.614% when pulling f9be62f on robertwb:nested-coders into 57f449c on apache:master.

@dhalperi
Copy link
Contributor

dhalperi commented May 3, 2017

There should probably be a JIRA and a discussion.

Do the other methods that take context (e.g., estimating byte size) need to be updated too?

@robertwb robertwb changed the title Split Coder's encode/decode methods into two methods depending on con… [BEAM-2166] Split Coder's encode/decode methods into two methods depending on context. May 4, 2017
@robertwb
Copy link
Contributor Author

robertwb commented May 4, 2017

JIRA filed. Added non-context versions of size estimation. Didn't add an Outer version, as these are estimations and context doesn't ever seem to be actually inspected for nearly any leaf Coders.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 70.604% when pulling 2dc6c86 on robertwb:nested-coders into 57f449c on apache:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 70.511% when pulling 2dc6c86 on robertwb:nested-coders into 57f449c on apache:master.

@lukecwik
Copy link
Member

lukecwik commented May 4, 2017

retest this please

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 70.507% when pulling 2dc6c86 on robertwb:nested-coders into 57f449c on apache:master.

@lukecwik
Copy link
Member

lukecwik commented May 4, 2017

LGTM

@asfgit asfgit closed this in d929300 May 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants