Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

Comments

TAJO-2146: Fragment interface cleanup#1015

Closed
jihoonson wants to merge 13 commits intoapache:masterfrom
jihoonson:TAJO-2146
Closed

TAJO-2146: Fragment interface cleanup#1015
jihoonson wants to merge 13 commits intoapache:masterfrom
jihoonson:TAJO-2146

Conversation

@jihoonson
Copy link
Contributor

No description provided.

@blrunner
Copy link
Contributor

Could you rebase it?

@jihoonson
Copy link
Contributor Author

Not ready for review.

@jihoonson
Copy link
Contributor Author

jihoonson commented May 14, 2016

I don't know why the changes in already committed patches are also shown.
The key changes are

  • Recaped Fragment to have a consistent interface.
  • Added FragmentSerdeHelper for user-defined fragment serialization and deserialization
  • Changed fragment configuration properties in storage-default.xml. A fragment and a fragment serde helper are specified for each tablespace like below.
<property>
  <name>tajo.storage.fragment.kind.file</name>
  <value>org.apache.tajo.storage.fragment.FileFragment</value>
</property>
<property>
  <name>tajo.storage.fragment.serde-helper.file</name>
  <value>org.apache.tajo.storage.fragment.FileFragmentSerdeHelper</value>
</property>
  • Removed RowFile.

@jihoonson
Copy link
Contributor Author

Now, ready for review.

* @param <F> Fragment class
* @param <P> Protocol Buffer Message class corresponding to the Fragment class
*/
public interface FragmentSerdeHelper<F extends Fragment, P extends Message> {
Copy link
Member

Choose a reason for hiding this comment

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

IMO, FragmentSerde seems to be a more proper name. As far as I know, Helper is mostly used for a utility class.

@hyunsik
Copy link
Member

hyunsik commented May 17, 2016

+1
The patch looks good to me. It's a nice cleanup for the fragment and its related ones. I left a trivial comment. If you agree, you can commit after reflecting the comment.

@asfgit asfgit closed this in 1c44272 May 18, 2016
@jihoonson
Copy link
Contributor Author

Thanks. I've changed the class name as you suggested.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants