HBASE-28006 Run spotless:apply on code base #120
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Requires #119 and demonstates changes due to the same |
NihalJain
commented
Aug 4, 2023
| /** | ||
| * hbase to kafka bridge. |
There was a problem hiding this comment.
Queue: 1) need to manually fix this with HBASE-28007
NihalJain
commented
Aug 4, 2023
| @@ -34,41 +32,20 @@ | |||
| import org.w3c.dom.NodeList; | |||
|
|
|||
| /** | |||
| * The topic routing/drop rules. | |||
There was a problem hiding this comment.
Queue: 2) need to manually fix this with HBASE-28007
NihalJain
commented
Aug 4, 2023
| @@ -75,16 +73,12 @@ | |||
| import org.apache.hbase.thirdparty.org.apache.commons.cli.CommandLine; | |||
|
|
|||
| /** | |||
| * Test Bulk Load and Spark on a distributed cluster. | |||
There was a problem hiding this comment.
Queue: 3) need to manually fix this with HBASE-28007
NihalJain
commented
Aug 4, 2023
| /** | ||
| * Allow the scan to go to replica, this would not affect the runCheck() |
There was a problem hiding this comment.
Queue: 4) need to manually fix this with HBASE-28007
NihalJain
commented
Aug 4, 2023
| @@ -35,19 +36,17 @@ | |||
| import org.apache.yetus.audience.InterfaceAudience; | |||
|
|
|||
| /** | |||
| * Run this example using command below: | |||
There was a problem hiding this comment.
Queue: 5) need to manually fix this with HBASE-28007
NihalJain
commented
Aug 4, 2023
| @@ -31,14 +30,16 @@ import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach, FunSuite} | |||
|
|
|||
| import scala.util.{Failure, Success, Try} | |||
|
|
|||
|
|
|||
| // Unit tests for HBASE-20521: change get configuration(TableOutputFormat.conf) object first sequence from jobContext to getConf | |||
There was a problem hiding this comment.
Queue: 6) need to manually fix this with HBASE-28007
Should write as multi-line javadoc
|
Skimmed over the changes due to 'mvn spotless:apply' and have identified/queued 6 changes which can be manually fixed as part of HBASE-28007 Please feel free to add more, if any. |
|
🎊 +1 overall
This message was automatically generated. |
Reidddddd
reviewed
Aug 8, 2023
dev-support/.scalafmt.conf
Outdated
| runner.dialect = scala213 | ||
| } | ||
| } | ||
| version = 3.7.12 |
|
Skimmed, lgtm. |
Reidddddd
reviewed
Aug 8, 2023
| this.avroWriter=avroWriter; | ||
| public KafkaTableForBridge(TableName tableName, Configuration conf, | ||
| TopicRoutingRules routingRules, Producer<byte[], byte[]> producer, | ||
| DatumWriter<HBaseKafkaEvent> avroWriter) { |
There was a problem hiding this comment.
One style nit, could we config style like these:
public KafkaTableForBridge(TableName tableName, Configuration conf,
TopicRoutingRules routingRules, Producer<byte[], byte[]> producer,
DatumWriter<HBaseKafkaEvent> avroWriter) {
this.conf = conf;
add one more indent for a newline (if any) in a method declaration
There was a problem hiding this comment.
Hi @Reidddddd this can be possibly solved but would require changing hbase_eclipse_formatter.xml. I have verified same issue exists in hbase main repo as well (as we have just copied hbase_eclipse_formatter.xml from there). We can try to fix that but I would say lets do it as a separate change as we may need to rerun spotless for all hbase repos after this change.
|
Refreshed by re-running (after making commit 2 changes of #119) |
|
🎊 +1 overall
This message was automatically generated. |
NihalJain
changed the title
|
Rebased the code and re-run CC: @Reidddddd, @wForget |
|
🎊 +1 overall
This message was automatically generated. |
Reidddddd
reviewed
Aug 11, 2023
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> |
There was a problem hiding this comment.
i'm fan of the old version, in two lines.
There was a problem hiding this comment.
Sure but this is how our current settings for pom formatting does (See in hbase repo: https://github.com/apache/hbase/blob/ab4b1d884f13d082a0906c3c3e9ec96bce7f3717/pom.xml#L1)
I have not tried to add any delta change in settings for java and scripts formatting and aligned the with hbase here?
Is it fine to take this as another change, across hbase project, where we tweak these settings globally?
Reidddddd
reviewed
Aug 11, 2023
| @@ -1,6 +1,5 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | |||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | |||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | |||
Reidddddd
reviewed
Aug 11, 2023
| @@ -1,6 +1,5 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | |||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | |||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | |||
Reidddddd
reviewed
Aug 11, 2023
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> |
Reidddddd
reviewed
Aug 11, 2023
| x => { | ||
| if(x._2.refCount < 0) { | ||
| connectionMap.foreach { x => | ||
| { |
There was a problem hiding this comment.
shouldn't be
x => {
why there is a new line here...
There was a problem hiding this comment.
Could set newlines.beforeCurlyLambdaParams = true to prevent this. Let me add this in our scalafmt. Let me if you want this as an addendum fix for pr-119 or part of this PR itself. For now added as 2nd commit along with spotless re-run with change.
Reidddddd
reviewed
Aug 11, 2023
| batchSize: Integer, | ||
| javaRdd: JavaRDD[T], | ||
| makeGet: Function[T, Get], | ||
| convertResult: Function[Result, U]): JavaRDD[U] = { |
There was a problem hiding this comment.
in fact, i feel the old way is much clearer to read...
but spotless... (shrug)
Reidddddd
reviewed
Aug 11, 2023
| import org.apache.hadoop.hbase.spark.datasources.Utils | ||
| import org.apache.hadoop.hbase.util.Bytes | ||
| import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach, FunSuite} | ||
|
|
||
| class StartsWithSuite extends FunSuite with | ||
| BeforeAndAfterEach with BeforeAndAfterAll with Logging { | ||
| class StartsWithSuite extends FunSuite with BeforeAndAfterEach with BeforeAndAfterAll with Logging { |
There was a problem hiding this comment.
Yes this one has 101 chars, maybe does not count braces. weird though
Reidddddd
reviewed
Aug 11, 2023
| extends FunSuite | ||
| with BeforeAndAfterEach | ||
| with BeforeAndAfterAll | ||
| with Logging { |
There was a problem hiding this comment.
while here they are multiple lines...
There was a problem hiding this comment.
don't understand the style spotless applies, seems doesn't align
There was a problem hiding this comment.
This is greater than 100 chars so I think it does like this.
class TableOutputFormatSuite extends FunSuite with BeforeAndAfterEach with BeforeAndAfterAll with Logging {
|
LGTM overall, left some comments and questions let's see whether there are other reviews |
|
🎊 +1 overall
This message was automatically generated. |
Reidddddd
approved these changes
Aug 11, 2023
Reidddddd
reviewed
Aug 11, 2023
| logError(s"Bug to be fixed: negative refCount of connection ${x._2}") | ||
| } | ||
| x => | ||
| { |
There was a problem hiding this comment.
have you re-run spotless:apply after the latest commit?
still a new line here
There was a problem hiding this comment.
yes i did this still does not go :(
|
Made another fix to re-order the imports, which was not there earlier. See commit 3: 965c41d Also added conifgs around lambda params newline behaviour, I think this one looks better than previous change. Let me raise an ADDENDUM for #119 and keep the format change separate. We can rebase and merge this once the new PR is done. |
|
Raised #121, lets merge this after that. Will rebase again. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.