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

ARROW-207: Extend BufferAllocator interface to allow decorators around BufferAllocator #81

Closed
wants to merge 1 commit into from

Conversation

vkorukanti
Copy link
Member

Currently AllocationManager needs BufferAllocator instance reference for accounting purposes.
Having a decorator around the BufferAllocator disturbs the accounting in AllocationManager.
Add an unwrap method to allow getting the reference to inner BufferAllocator instance and use
this method in AllocationManager to get the actual BufferAllocator.

/**

  • Unwrap the class so that exposes the provided interface, if possible. Otherwise, throw Exception.
  • @param c
  •      The class or interface that you want this class to implement/extend.
    
  • @return The instance of that class related to 'this'
    */
    T unwrap(Class c);

@StevenMPhillips: Could you please review this patch?

…d BufferAllocator

Currently AllocationManager needs BufferAllocator instance reference for accounting purposes.
Having a decorator around the BufferAllocator distrubs the accounting in AllocationManager.
Add an unwrap method to allow getting the reference to inner BufferAllocator instance and use
this method in AllocationManager to get the actual BufferAllocator.

  /**
   * Unwrap the class so that exposes the provided interface, if possible. Otherwise, throw Exception.
   * @param c
   *          The class or interface that you want this class to implement/extend.
   * @return The instance of that class related to 'this'
   */
  <T> T unwrap(Class<T> c);
@StevenMPhillips
Copy link
Contributor

If AllocationManager assumes the the BufferAllocator is a specific implementation class, what is the point of the interface? It seems to me we should either fix the interface or get rid of it.

@vkorukanti
Copy link
Member Author

I agree, current AllocationManager depends on the implementation of BufferAllocator, BaseAllocator, than the interface.

@jacques-n
Copy link
Contributor

I suggest we abandon this and come up with a different approach for the extensibility you're trying to achieve.

@vkorukanti vkorukanti closed this Jul 7, 2016
praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Aug 30, 2018
1. Added lock to holder read to address potential race condition.
2. Fixed log message.
3, Addressed breaking arrow change.
praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Aug 30, 2018
1. Added lock to holder read to address potential race condition.
2. Fixed log message.
3, Addressed breaking arrow change.
praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Aug 30, 2018
1. Added lock to holder read to address potential race condition.
2. Fixed log message.
3, Addressed breaking arrow change.
praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Aug 30, 2018
1. Added lock to holder read to address potential race condition.
2. Fixed log message.
3, Addressed breaking arrow change.
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 2, 2018
Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com>

Closes apache#81 from asandryh/PARQUET-568 and squashes the following commits:

f619ed0 [Aliaksei Sandryhaila] Addressed PR comments.
bf12164 [Aliaksei Sandryhaila] Added column selection capability to parquet_reader.
praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Sep 4, 2018
1. Added lock to holder read to address potential race condition.
2. Fixed log message.
3, Addressed breaking arrow change.
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 4, 2018
Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com>

Closes apache#81 from asandryh/PARQUET-568 and squashes the following commits:

f619ed0 [Aliaksei Sandryhaila] Addressed PR comments.
bf12164 [Aliaksei Sandryhaila] Added column selection capability to parquet_reader.

Change-Id: I5cb658f51e9f761e83be22424f2f36593a169766
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 6, 2018
Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com>

Closes apache#81 from asandryh/PARQUET-568 and squashes the following commits:

f619ed0 [Aliaksei Sandryhaila] Addressed PR comments.
bf12164 [Aliaksei Sandryhaila] Added column selection capability to parquet_reader.

Change-Id: I5cb658f51e9f761e83be22424f2f36593a169766
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 7, 2018
Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com>

Closes apache#81 from asandryh/PARQUET-568 and squashes the following commits:

f619ed0 [Aliaksei Sandryhaila] Addressed PR comments.
bf12164 [Aliaksei Sandryhaila] Added column selection capability to parquet_reader.

Change-Id: I5cb658f51e9f761e83be22424f2f36593a169766
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 8, 2018
Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com>

Closes apache#81 from asandryh/PARQUET-568 and squashes the following commits:

f619ed0 [Aliaksei Sandryhaila] Addressed PR comments.
bf12164 [Aliaksei Sandryhaila] Added column selection capability to parquet_reader.

Change-Id: I5cb658f51e9f761e83be22424f2f36593a169766
praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Sep 10, 2018
1. Added lock to holder read to address potential race condition.
2. Fixed log message.
3, Addressed breaking arrow change.
praveenbingo added a commit to praveenbingo/arrow that referenced this pull request Sep 10, 2018
1. Added lock to holder read to address potential race condition.
2. Fixed log message.
3, Addressed breaking arrow change.
zhouyuan pushed a commit to zhouyuan/arrow that referenced this pull request Nov 18, 2020
* remove numa, vmemcache dependency

* remove properties test
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.

3 participants