Skip to content

[DO NOT Merge] Very initial pass - ByteBuffer -> Memory#3915

Closed
niketh wants to merge 1 commit intoapache:masterfrom
niketh:memory
Closed

[DO NOT Merge] Very initial pass - ByteBuffer -> Memory#3915
niketh wants to merge 1 commit intoapache:masterfrom
niketh:memory

Conversation

@niketh
Copy link
Copy Markdown
Contributor

@niketh niketh commented Feb 7, 2017

@cheddar @leventov This is a very initial pass at moving ByteBuffer -> Memory.

  1. Introduced PositionMemoryRegion
  2. Made changes to GenericIndexed
  3. Made changes to FileSmoosher.
  4. Trying to read data from Memory in indexv9loader

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This method should be a part of Memory interface.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ints.checkedCast

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it should exist, should be a part of the memory library, not Druid

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Memory should be Closeable or AutoCloseable

Comment thread java-util/pom.xml Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In #3915 we came to agreement that Memory should be in it's small dedicated library, not datasketches

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought we would like to keep Memory with DataSketches for now. I don't remember us concluding that we need to move it. What do you think @leventov @cheddar @leerho

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@niketh sorry, wrong link. @gianm suggested that here: #3892 (comment) and I agreed here: #3892 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't agreed. And @leventov and I are busy working out a solution that should work for both DataSketches and Druid. Let's not jump to conclusions before we determine that a fork is the only solution.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@leerho it is already done since apache/datasketches-java@92e67a9. and the dependency is already com.yahoo.datasketches:memory, that I missed when wrote my previous comments in this small thread.

@niketh
Copy link
Copy Markdown
Contributor Author

niketh commented Mar 1, 2017

@cheddar @leventov @gianm @fjy @leerho @akashdw Part 2 of the ByteBuffer -> Memory changes. There are only a few more changes left and we should have a version that will be compatible with already persisted segments. I will be posting Part 3 later this week, which should be the final one (sans code review refactoring)

@leventov
Copy link
Copy Markdown
Member

leventov commented Mar 1, 2017

@niketh what's the point of doing this with the old API?

@weijietong
Copy link
Copy Markdown

@niketh is this PR still underway ? please consider #3716 while doing this work .

@niketh
Copy link
Copy Markdown
Contributor Author

niketh commented Mar 24, 2017

@leventov @leerho @cheddar @gianm Lee has finalized the new memory interfaces yesterday. Working on moving the old Memory to the new Memory interfaces.

@drcrallen drcrallen added the WIP label Aug 21, 2017
@stale
Copy link
Copy Markdown

stale Bot commented Feb 28, 2019

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

@stale stale Bot added the stale label Feb 28, 2019
@stale
Copy link
Copy Markdown

stale Bot commented Mar 7, 2019

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@stale stale Bot closed this Mar 7, 2019
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.

5 participants