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.

Sign up for GitHub

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

Jump to bottom

DRILL-8454: Disable unsupported MapR profile and plugin #2825

Merged
merged 1 commit into from Oct 21, 2023
Merged

DRILL-8454: Disable unsupported MapR profile and plugin #2825

merged 1 commit into from Oct 21, 2023

Conversation

jnturton
Copy link
Contributor

DRILL-8454: Disable unsupported MapR profile and plugin

Description

The MapR build profile and format plugin, which fell out of support in the open source Drill codebase years ago, are disabled. Drill users needing support for these components should contact HPE.

Documentation

See the HPE Ezmeral docs.

Testing

Build and unit tests of remaining modules pass.

@jnturton jnturton self-assigned this Aug 24, 2023
@jnturton jnturton requested a review from rymarm August 24, 2023 13:35
@jnturton jnturton force-pushed the 8454-disable-mapr branch 2 times, most recently from 1dbf760 to 8fc3b8b Compare August 25, 2023 06:23
@jnturton
Copy link
Contributor Author

This one's ready for review.

@cgivre cgivre requested a review from Agirish August 25, 2023 14:52
@jnturton
Copy link
Contributor Author

jnturton commented Aug 28, 2023 •
edited

A note to the reviewers. Git's default diff algorithm makes it very hard to see that the changes this PR makes to pom.xml are simply whole sections moving into comment blocks. GitHub does not allow users to select the diff algorithm at this time so, to ease review, I have therefore uploaded the diff of pom.xml generated using git diff --diff-algorithm=patience separately as a Gist. It is of course also possible to produce the same locally after pulling down my branch.

@rymarm
Copy link
Member

rymarm commented Sep 4, 2023

@jnturton Hi! I'll take a look at this pull request this week later or even next week. I've just got back from a vacation and got on a "burning boat" of work)

But I have a question now, Why did you decide to comment the mapr specific changes, but not completely remove them?

And in addition, I suppose that we also need to remove mentions of maprdb support from the documentation or leave a link to HPE documentation.

@jnturton
Copy link
Contributor Author

jnturton commented Sep 4, 2023

@rymarm welcome back! I wasn't sure whether to delete or comment out the mapr modules. I decided to try moving them to new, commented out sections of "Objects of type foo that are no longer supported" that seemed to keep an informative history in the source files without a lot of extra cruft. But that history is always available in Git so I'm also happy to delete instead...

cgivre
cgivre approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@cgivre cgivre left a comment

Choose a reason for hiding this comment

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

LGTM +1
In general I don't like leaving commented out code blocks, but in the event that someone wants to use a current version of Drill with MapR, I do think it is fine to leave them for a while.

@cgivre
Copy link
Contributor

cgivre commented Oct 12, 2023

@jnturton Can we merge this?

@jnturton
Copy link
Contributor Author

@cgivre let me convert the comment blocks to deletions as suggested by you and @rymarm and then let's merge it.

@jnturton jnturton mentioned this pull request Oct 19, 2023
Closed
@jnturton
Copy link
Contributor Author

@cgivre let me convert the comment blocks to deletions as suggested by you and @rymarm and then let's merge it.

Done...

@jnturton jnturton merged commit 25d3774 into apache:master Oct 21, 2023
8 checks passed
cgivre pushed a commit to cgivre/drill that referenced this pull request Nov 2, 2023
@jnturton @cgivre
Disable unsupported MapR profile and plugins. (apache#2825)
f41eb03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgivre cgivre cgivre approved these changes

@vvysotskyi vvysotskyi Awaiting requested review from vvysotskyi

@luocooong luocooong Awaiting requested review from luocooong

@rymarm rymarm Awaiting requested review from rymarm

@Agirish Agirish Awaiting requested review from Agirish

Assignees

@jnturton jnturton

Labels
code-cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jnturton @rymarm @cgivre