Skip to content

Conversation

@NihalJain
Copy link
Contributor

@NihalJain NihalJain commented Aug 4, 2023

  • Added the base plugin to pom.xml (similar to other hbase repos)
  • Extended the plugin to support formatting of scala code / headers
  • Added the files required by the base plugin ( copying them from hbase repo)
  • Added a new file .scalafmt.conf, which is required by spotless for scala (copied from spark repo)
  • Made changes to fix docstring param new line issue and used latest scalafmt version i.e. 3.7.12

@NihalJain
Copy link
Contributor Author

Commit 1: Added plug-in along with required files (including support for scala code)
Commit 2:
Ran mvn spotless:apply and staged all file changes. We can decide if we want to do this as a separate change to keep things clean.

Let me know what do you all think of this change.

CC: @duo @ndimiduk @wchevreuil @petersomogyi @Reidddddd

* The function performing real filtering operations. The format of filterBytes depends on the
* implementation of the BytesEncoder.
*
* @param input:
Copy link
Contributor Author

@NihalJain NihalJain Aug 4, 2023

Choose a reason for hiding this comment

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

this does not look right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its an issue with scalafmt plugin version we are using: scalameta/scalafmt#1387
Will try to bump up to fixed version and update here: scalameta/scalafmt#1987 or apply workaround (as last resort)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: bumping did not work, so had to apply workaround

@Reidddddd
Copy link

Commit 1: Added plug-in along with required files (including support for scala code) Commit 2: Ran mvn spotless:apply and staged all file changes. We can decide if we want to do this as a separate change to keep things clean.

Let me know what do you all think of this change.

CC: @duo @ndimiduk @wchevreuil @petersomogyi @Reidddddd

duo is this account: @Apache9

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 26s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 1s No case conflicting files found.
+0 🆗 markdownlint 0m 1s markdownlint was not available.
+0 🆗 shelldocs 0m 1s Shelldocs was not available.
+0 🆗 spotbugs 0m 0s spotbugs executables are not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 20 new or modified test files.
_ master Compile Tests _
+0 🆗 mvndep 0m 27s Maven dependency ordering for branch
+1 💚 mvninstall 1m 45s master passed
+1 💚 compile 1m 1s master passed
+1 💚 javadoc 0m 57s master passed
+1 💚 scaladoc 0m 0s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 7s Maven dependency ordering for patch
+1 💚 mvninstall 0m 53s the patch passed
+1 💚 compile 1m 12s the patch passed
+1 💚 javac 1m 12s the patch passed
+1 💚 scalac 1m 12s the patch passed
+1 💚 shellcheck 0m 0s There were no new shellcheck issues.
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 xml 0m 10s The patch has no ill-formed XML file.
+1 💚 javadoc 1m 0s the patch passed
+1 💚 scaladoc 0m 0s the patch passed
_ Other Tests _
+1 💚 unit 9m 3s root in the patch passed.
19m 29s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-119/1/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR #119
Optional Tests dupname markdownlint xml shellcheck shelldocs javac javadoc unit compile spotbugs findbugs scalac scaladoc
uname Linux ff88f39620be 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 GNU/Linux
Build tool hb_maven
Personality dev-support/jenkins/hbase-personality.sh
git revision master / bfcdf68
Default Java Oracle Corporation-1.8.0_282-b08
Test Results https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-119/1/testReport/
Max. process+thread count 954 (vs. ulimit of 12500)
modules C: kafka kafka/hbase-kafka-model kafka/hbase-kafka-proxy spark spark/hbase-spark-protocol spark/hbase-spark-protocol-shaded spark/hbase-spark spark/hbase-spark-it hbase-connectors-assembly . test-reporting U: .
Console output https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-119/1/console
versions git=2.20.1 shellcheck=0.5.0
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Reidddddd
Copy link

if we want to do this as a separate change to keep things clean.

I'd like the separation

@NihalJain
Copy link
Contributor Author

NihalJain commented Aug 4, 2023

if we want to do this as a separate change to keep things clean.

I'd like the separation

Sure will force push and drop 2nd commit once it looks good to others. We can treat (commit 2 and any new ones) for ref until everyone is fine with changes

@NihalJain NihalJain force-pushed the HBASE-27178 branch 2 times, most recently from c3d411f to 4b9553c Compare August 4, 2023 10:01
@NihalJain
Copy link
Contributor Author

NihalJain commented Aug 4, 2023

Reverted 2nd commit and fixed scala param issue. The changes

Commit 1: Added plug-in along with required files (including support for scala code) Commit 2: Ran mvn spotless:apply and staged all file changes. We can decide if we want to do this as a separate change to keep things clean.

Let me know what do you all think of this change.

CC: @duo @ndimiduk @wchevreuil @petersomogyi @Reidddddd

Dropped 2nd commit as discuused with @Reidddddd. Will raise a WIP-PR with results after applying commit 1 here and running 'mvn spotless:apply'.

runner.dialect = scala213
}
}
version = 3.7.12 No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

nit: end with newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 35s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 shelldocs 0m 0s Shelldocs was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
-0 ⚠️ test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+1 💚 mvninstall 1m 40s master passed
+1 💚 compile 1m 16s master passed
+1 💚 javadoc 1m 28s master passed
_ Patch Compile Tests _
+1 💚 mvninstall 1m 5s the patch passed
+1 💚 compile 1m 11s the patch passed
+1 💚 javac 1m 11s the patch passed
+1 💚 shellcheck 0m 0s There were no new shellcheck issues.
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 xml 0m 2s The patch has no ill-formed XML file.
+1 💚 javadoc 1m 5s the patch passed
_ Other Tests _
+1 💚 unit 9m 28s root in the patch passed.
19m 58s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-119/2/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR #119
Optional Tests dupname xml shellcheck shelldocs javac javadoc unit compile
uname Linux 6dbfd1e806e7 5.4.0-152-generic #169-Ubuntu SMP Tue Jun 6 22:23:09 UTC 2023 x86_64 GNU/Linux
Build tool hb_maven
Personality dev-support/jenkins/hbase-personality.sh
git revision master / bfcdf68
Default Java Oracle Corporation-1.8.0_282-b08
Test Results https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-119/2/testReport/
Max. process+thread count 959 (vs. ulimit of 12500)
modules C: . U: .
Console output https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-119/2/console
versions git=2.20.1 shellcheck=0.5.0
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

docstrings.style = Asterisk
# See https://github.com/scalameta/scalafmt/issues/1387
docstrings.wrap = no
maxColumn = 98
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need feedback: should we make this 100, similar to setting that we have for java? Or keep it same as spark project?

Choose a reason for hiding this comment

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

let's align with hbase project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in commit 2

docstrings.wrap = no
maxColumn = 98
runner.dialect = scala212
fileOverride {
Copy link
Contributor Author

@NihalJain NihalJain Aug 4, 2023

Choose a reason for hiding this comment

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

Need feedback: IMHO we can drop this as we do not have this directory/muliple scala implementations similar to how spark has: https://github.com/apache/spark/tree/master/core/src/main/scala-2.13

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in commit 2

@NihalJain NihalJain requested review from Reidddddd and wForget August 8, 2023 19:31
@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 29s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 shelldocs 0m 0s Shelldocs was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
-0 ⚠️ test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+1 💚 mvninstall 1m 33s master passed
+1 💚 compile 0m 54s master passed
+1 💚 javadoc 0m 51s master passed
_ Patch Compile Tests _
+1 💚 mvninstall 0m 49s the patch passed
+1 💚 compile 0m 52s the patch passed
+1 💚 javac 0m 52s the patch passed
+1 💚 shellcheck 0m 0s There were no new shellcheck issues.
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 xml 0m 2s The patch has no ill-formed XML file.
+1 💚 javadoc 0m 51s the patch passed
_ Other Tests _
+1 💚 unit 8m 56s root in the patch passed.
17m 15s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-119/3/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR #119
Optional Tests dupname xml shellcheck shelldocs javac javadoc unit compile
uname Linux 0152dd29d01b 5.4.0-152-generic #169-Ubuntu SMP Tue Jun 6 22:23:09 UTC 2023 x86_64 GNU/Linux
Build tool hb_maven
Personality dev-support/jenkins/hbase-personality.sh
git revision master / bfcdf68
Default Java Oracle Corporation-1.8.0_282-b08
Test Results https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-119/3/testReport/
Max. process+thread count 959 (vs. ulimit of 12500)
modules C: . U: .
Console output https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-119/3/console
versions git=2.20.1 shellcheck=0.5.0
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Reidddddd
Copy link

let's wait some time whether there are other comments

@Reidddddd Reidddddd merged commit c1eb13a into apache:master Aug 11, 2023
@NihalJain
Copy link
Contributor Author

Thanks for review / merge. @Reidddddd / @wForget
I have rebased #120 with this change.

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.

4 participants