Skip to content

[improve][sql] Remove unnecessary future encapsulation#19959

Merged
tisonkun merged 4 commits into
apache:masterfrom
Technoboy-:improve-sql-schema-provider
Apr 5, 2023
Merged

[improve][sql] Remove unnecessary future encapsulation#19959
tisonkun merged 4 commits into
apache:masterfrom
Technoboy-:improve-sql-schema-provider

Conversation

@Technoboy-

@Technoboy- Technoboy- commented Mar 29, 2023

Copy link
Copy Markdown
Contributor

Motivation

Remove unnecessary JDK future encapsulation, using async method instead.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@Technoboy- Technoboy- added area/sql Pulsar SQL related features ready-to-test labels Mar 29, 2023
@Technoboy- Technoboy- added this to the 3.0.0 milestone Mar 29, 2023
@Technoboy- Technoboy- self-assigned this Mar 29, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Mar 29, 2023
tisonkun
tisonkun approved these changes Mar 29, 2023
View reviewed changes

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks for your improvement!

@Technoboy- Technoboy- closed this Mar 31, 2023
@Technoboy- Technoboy- reopened this Mar 31, 2023
@codecov-commenter

codecov-commenter commented Mar 31, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.86%. Comparing base (8c50a6c) to head (6cf3b46).
⚠️ Report is 2672 commits behind head on master.

Files with missing lines Patch % Lines
...pulsar/sql/presto/PulsarSqlSchemaInfoProvider.java 85.71% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19959       +/-   ##
=============================================
+ Coverage     33.17%   72.86%   +39.68%     
- Complexity    12158    31606    +19448     
=============================================
  Files          1499     1861      +362     
  Lines        113832   137304    +23472     
  Branches      12368    15124     +2756     
=============================================
+ Hits          37769   100040    +62271     
+ Misses        71143    29312    -41831     
- Partials       4920     7952     +3032
Flag Coverage Δ
inttests 24.39% <ø> (?)
systests 24.97% <ø> (?)
unittests 72.16% <85.71%> (+38.98%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...pulsar/sql/presto/PulsarSqlSchemaInfoProvider.java 68.00% <85.71%> (ø)

... and 1532 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tisonkun

tisonkun commented Mar 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Tests failed on change:

  Error:  testGetSchemaInfo(org.apache.pulsar.sql.presto.TestPulsarRecordCursor)  Time elapsed: 0.19 s  <<< FAILURE!
  java.lang.reflect.InvocationTargetException
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
  	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
  	at org.apache.pulsar.sql.presto.TestPulsarRecordCursor.testGetSchemaInfo(TestPulsarRecordCursor.java:495)
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
  	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
  	at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:139)
  	at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:677)
  	at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:221)
  	at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
  	at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:969)
  	at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:194)
  	at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:148)
  	at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
  	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
  	at org.testng.TestRunner.privateRun(TestRunner.java:829)
  	at org.testng.TestRunner.run(TestRunner.java:602)
  	at org.testng.SuiteRunner.runTest(SuiteRunner.java:437)
  	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:431)
  	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:391)
  	at org.testng.SuiteRunner.run(SuiteRunner.java:330)
  	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
  	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:95)
  	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1256)
  	at org.testng.TestNG.runSuitesLocally(TestNG.java:1176)
  	at org.testng.TestNG.runSuites(TestNG.java:1099)
  	at org.testng.TestNG.run(TestNG.java:1067)
  	at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
  	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeMulti(TestNGDirectoryTestSuite.java:193)
  	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:94)
  	at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
  	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
  	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
  	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
  	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
  Caused by: com.google.common.cache.CacheLoader$InvalidCacheLoadException: CacheLoader returned null for key \x00\x00\x00\x00\x00\x00\x00\x00.
  	at com.google.common.cache.LocalCache$Segment.getAndRecordStats(LocalCache.java:2319)
  	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2283)
  	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2159)
  	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2049)
  	at com.google.common.cache.LocalCache.get(LocalCache.java:3966)
  	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:3989)
  	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4950)
  	at org.apache.pulsar.sql.presto.PulsarSqlSchemaInfoProvider.getSchemaByVersion(PulsarSqlSchemaInfoProvider.java:72)
  	at org.apache.pulsar.sql.presto.PulsarRecordCursor.getSchemaInfo(PulsarRecordCursor.java:661)
  	... 38 more

@tisonkun

tisonkun commented Mar 31, 2023

Copy link
Copy Markdown
Member

I push a fix at cedb6a5.

@tisonkun

tisonkun commented Apr 5, 2023

Copy link
Copy Markdown
Member

Merging...

@tisonkun tisonkun merged commit f568c8f into apache:master Apr 5, 2023
@Technoboy- Technoboy- deleted the improve-sql-schema-provider branch November 11, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tisonkun tisonkun tisonkun approved these changes

Assignees

@Technoboy- Technoboy-

Labels

area/sql Pulsar SQL related features doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Technoboy- @codecov-commenter @tisonkun