[go: nahoru, domu]
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

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

Merged
merged 4 commits into from
Apr 5, 2023
Merged

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

merged 4 commits into from
Apr 5, 2023

Conversation

Technoboy-
Copy link
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
Copy link
Member
@tisonkun tisonkun left a comment

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
Copy link
codecov-commenter commented Mar 31, 2023 •
edited
Loading

Codecov Report

Merging #19959 (8aea9a2) into master (7a99e74) will increase coverage by 5.99%.
The diff coverage is 5.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19959      +/-   ##
============================================
+ Coverage     31.88%   37.88%   +5.99%     
+ Complexity     6421      618    -5803     
============================================
  Files          1682     1684       +2     
  Lines        127354   130256    +2902     
  Branches      13892    14509     +617     
============================================
+ Hits          40601    49341    +8740     
+ Misses        80714    74523    -6191     
- Partials       6039     6392     +353
Flag Coverage Δ
inttests 24.65% <0.58%> (+0.14%) ⬆️
systests 25.29% <2.35%> (+0.20%) ⬆️
unittests 33.16% <4.41%> (+15.89%) ⬆️

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

Impacted Files Coverage Δ
...r/delayed/bucket/BucketDelayedDeliveryTracker.java 0.00% <0.00%> (ø)
...layed/bucket/CombinedSegmentDelayedIndexQueue.java 0.00% <0.00%> (ø)
...dbalance/extensions/ExtensibleLoadManagerImpl.java 3.07% <0.00%> (+0.37%) ⬆️
.../loadbalance/extensions/data/BrokerLookupData.java 0.00% <0.00%> (ø)
...ommon/policies/data/stats/ReplicatorStatsImpl.java 0.00% <0.00%> (ø)
...sar/common/policies/data/stats/TopicStatsImpl.java 52.19% <0.00%> (+17.71%) ⬆️
...ache/pulsar/broker/namespace/NamespaceService.java 44.14% <20.83%> (+7.77%) ⬆️
...pache/pulsar/broker/admin/impl/NamespacesBase.java 32.24% <66.66%> (+7.73%) ⬆️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 46.64% <100.00%> (+6.83%) ⬆️

... and 373 files with indirect coverage changes

@tisonkun
Copy link
Member
tisonkun commented Mar 31, 2023 •
edited
Loading

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
Copy link
Member

I push a fix at cedb6a5.

@tisonkun
Copy link
Member
tisonkun commented Apr 5, 2023

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