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

Adding support for Oracle Cloud Infrastructure (OCI) Object Store as Delta Storage #468

Closed
wants to merge 7 commits into from

Conversation

vibhaska
Copy link
Contributor

@vibhaska vibhaska commented Jul 1, 2020

Adding support for Oracle Cloud Infrastructure (OCI) Object Store as Delta Storage by introducing OCILogStore.

Regarding Storage configuration page in Delta Documentation, I request following changes mentioned here.

@pranavanand
Copy link
Contributor

Thanks for submitting this PR! We are currently coming up with a process for contributing third-party support and will let you know once that's ready.

@ksranga
Copy link

ksranga commented Jul 12, 2020

Can this be ported to delta 0.6.x version so its available for spark 2.x as well?

@vibhaska
Copy link
Contributor Author

@ksranga Yes.

I guess only one line change is required: Remove line number 268 from LogStoreSuite.scala. If you want I can raise another PR for old branch after testing and verification.

@vibhaska
Copy link
Contributor Author

Thanks for submitting this PR! We are currently coming up with a process for contributing third-party support and will let you know once that's ready.

@pranavanand can you please share any further update.

@ddraj
Copy link

ddraj commented Dec 22, 2020

Echoing @vibhaska's question .. Hi @pranavanand, any chance the PR can be merged? Could you please let us know if anything is pending on the PR that we should address?

@tdas
Copy link
Contributor

tdas commented Apr 21, 2021

Hey @ddraj

Our apologies for sitting on the PR for so long. We wanted to figure out what is the best we integrate slightly less battle-tested features into our repo, and we finally figured it out. We are currently in the process of converting our repo to a multi-module SBT project so that we can add other sub-modules (mapping to other maven release artifacts beyond delta-core). Then we can add a contribs module and then accept such new and experimental features.

Are you still interested in contributing to this feature?

@ddraj
Copy link

ddraj commented Apr 23, 2021

Hey @ddraj

Our apologies for sitting on the PR for so long. We wanted to figure out what is the best we integrate slightly less battle-tested features into our repo, and we finally figured it out. We are currently in the process of converting our repo to a multi-module SBT project so that we can add other sub-modules (mapping to other maven release artifacts beyond delta-core). Then we can add a contribs module and then accept such new and experimental features.

Are you still interested in contributing to this feature?

Thanks @tdas. Yes we are still interested. Please let us know if we need to do anything specific.

@tdas
Copy link
Contributor

tdas commented Apr 23, 2021

So the refactor to multi-module has been merged. Lets' work towards getting this in. Here are the next steps.

  1. Please define a new module using the same pattern as the core module in the build.sbt file.

    • Does the IBM support need additional maven dependencies? I am guessing not. In that case, we can make the module name generic like "delta-contribs" (that is, not specific to IBM). Other such zero-dependency contributions can go in that project as well. Otherwise, lets put it in a specifically named module like "delta-contribs-ibm"
  2. Move the new log store code inside the <new-module-dir>/src/main/<package>/... and the test code in a separate file in <new-module-dir>/src/test/<package>/....

Let me know if you need help to get it working.

Vivek Bhaskar added 2 commits April 26, 2021 13:10
…Delta Storage by introducing OCILogStore.

https://developercertificate.org/ Signed-off-by: Vivek Bhaskar <vivek.bhaskar@oracle.com>
…Delta Storage by introducing OCILogStore.

https://developercertificate.org/ Signed-off-by: Vivek Bhaskar <vivek.bhaskar@oracle.com>
@vibhaska
Copy link
Contributor Author

vibhaska commented Apr 26, 2021

@tdas I have updated the changes as suggested. Kindly check if this is what you wanted.

Couple of points where I need some guidance:

  1. Will there be any root project, and jars from same will be published? Or will a separate jars be published from this module. I am not clear how.
  2. I am also not clear why CI failed. Is this due to multi module nature?

@tdas
Copy link
Contributor

tdas commented May 6, 2021

These are good questions. My apologies for the delay, we have been trying to line up a few more features like a new, more stable LogStore API that LogStore builders like you folks can use. However, there are a few more pieces that we still need to get in for the public API to be easily usable. I dont want you folks to block on those. So we can continue working using the existing LogStore API, which we will continue supporting for at least the 1.x releases. Later we can rewrite the OCILogStore to use the new API, we are not going to worry about the 1.0 release we are targeting in a couple of weeks.

Now back to the question. We will be releasing a separate Maven artifact for each subproject. The refactoring you have done in the sbt looks good at a high-level. I will leave more detailed comments on them soon. I will also take a look at why the tests failed.

@tdas
Copy link
Contributor

tdas commented May 6, 2021

Somehow the sbt download failed. Could be a random error. Can you run the tests once again. An easy to rerun tests is to simply push an empty commit to this branch.

build.sbt Outdated
// mimaSettings,
unidocSettings,
releaseSettings,
libraryDependencies ++= Seq(
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to add these dependencies? I think this project automatically gets the dependencies from the core module

build.sbt Outdated
commonSettings,
scalaStyleSettings,
// mimaSettings,
unidocSettings,
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think we need unidoc. there is no public in this project yet.

@tdas
Copy link
Contributor

tdas commented May 6, 2021

The community has already created the contrib submodule! So all you have to do merge with the master and add your LogStore to the existing directories. No need to fix anything in the build.sbt!

@tdas
Copy link
Contributor

tdas commented May 7, 2021

@vibhaska @ddraj if you have time to update the PR, please do so quickly as we preparing to release 1.0 soon. Alternatively, if you are okay, I can do the slight refactoring as well (you still get commit authorship).

@ddraj
Copy link

ddraj commented May 7, 2021

@tdas coincidentally @vibhaska is on vacation this week and the next. So we are not able to update the PR :( I'll happily take your offer for the slight updates. Thanks a lot @tdas

@tdas
Copy link
Contributor

tdas commented May 7, 2021

@ddraj all right! I will do it. Do you mind if I change the name OracleCILogstore to make it more obvious which cloud it is? Similar to our naming of IBMCOSLogStore and AzureLogStore.

@vibhaska
Copy link
Contributor Author

vibhaska commented May 8, 2021

Sorry for the delayed response. I have checked-in refactoring changes. I have also updated the file name, as suggested.

@tdas Thanks for offering help with the changes. Kindly review and please suggest if any more changes are required.

@ddraj Thanks for your support.

@tdas
Copy link
Contributor

tdas commented May 10, 2021

@vibhaska thank you for the update. One minor correction I suggest is to rename OracleOCI to just OracleCI because the O in OCI is redundant with Oracle. I can do rename super easily if that's okay with you.

@ddraj
Copy link

ddraj commented May 10, 2021

@tdas any chance we can name it as OracleCloudLogStore. I think that would fully convey the purpose of this implementation.

@tdas
Copy link
Contributor

tdas commented May 10, 2021

Sure. As long as there won't be any other variation of LogStore needed on the Oracle Cloud, maybe for any other storage product in Oracle Cloud.

@ddraj
Copy link

ddraj commented May 11, 2021

Don’t think that’s in the offing anytime soon @tdas Lets rename it to OracleCloudLogStore

@tdas
Copy link
Contributor

tdas commented May 11, 2021

Alright!

@vibhaska
Copy link
Contributor Author

@tdas @ddraj Updated the file name, as suggested.

@vibhaska
Copy link
Contributor Author

@tdas Kindly suggest if any further changes are required.

@ddraj
Copy link

ddraj commented May 13, 2021

@tdas echoing what @vibhaska asked. Is there anything you need from us?

@ddraj
Copy link

ddraj commented May 18, 2021

Hi @tdas gentle ping :)

@tdas tdas closed this in cb46fd1 May 18, 2021
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.

6 participants