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

[CARBONDATA-3889] Cleanup code for carbondata-streaming module #3826

Closed
wants to merge 2 commits into from

Conversation

QiangCai
Copy link
Contributor

@QiangCai QiangCai commented Jul 4, 2020

Why is this PR needed?

need cleanup code in carbondata-streaming module

What changes were proposed in this PR?

Cleanup code in carbondata-streaming module

Does this PR introduce any user interface change?

  • No
  • Yes. (please explain the change and update document)

Is any new testcase added?

  • No
  • Yes

@CarbonDataQA1
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1566/

@CarbonDataQA1
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3303/

@QiangCai QiangCai changed the title [CARBONDATA-3889] Cleanup code in carbondata-streaming module [CARBONDATA-3889] Cleanup code for carbondata-streaming module Jul 7, 2020
} else {
LOGGER.error(
"Not able to acquire the lock for stream table status updation for table " + table
"Not able to acquire the lock for stream table status update for table " + table
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Not able to acquire the lock for stream table status update for table " + table
"Not able to acquire the status update lock for streaming table " + table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -95,7 +95,7 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>2.18</version>
<!-- Note config is repeated in scalatest config -->
<!-- Note config is repeated in scala test config -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add license header in this POM file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

return CarbonTablePath.isCarbonDataFile(file.getName());
}
});
return carbonDir.listFiles(file -> CarbonTablePath.isCarbonDataFile(file.getName()));
Copy link
Member

Choose a reason for hiding this comment

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

Are we target on jdk8 from now on? If yes, codes in org.apache.carbondata.common like Maps, Strings can be removed

Copy link
Contributor Author

@QiangCai QiangCai Jul 17, 2020

Choose a reason for hiding this comment

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

For carbon 2.0, the answer is yes, in my opinion.

we already used new features of jdk8(lambda, stream API)

Copy link
Member

Choose a reason for hiding this comment

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

OK.

FYI: I check spark release note, support for Java 7 was removed since 2.2

@CarbonDataQA1
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1672/

@CarbonDataQA1
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3414/

@kevinjmh
Copy link
Member

LGTM

@asfgit asfgit closed this in 23e2760 Jul 19, 2020
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.

None yet

4 participants