Remove deprecated datasources config property #306
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
|
@electrum . Can you please review them ? |
kokosing
reviewed
Feb 25, 2019
There was a problem hiding this comment.
There are tests failures, would you please take a look:
ERROR] TestResourceGroupDbIntegration.testMemoryFraction:28 » Creation Unable to crea...
[ERROR] TestMemoryManager.testClusterPools:253->createQueryRunner:385 » Creation Unabl...
[ERROR] TestMemoryManager.testNoLeak:216->testNoLeak:227->createQueryRunner:385 » Creation
[ERROR] TestMemoryManager.testOutOfMemoryKiller:121->createQueryRunner:385 » Creation ...
[ERROR] TestMemoryManager.testQueryMemoryPerNodeLimit:377->createQueryRunner:385 » Creation
[ERROR] TestMemoryManager.testQueryTotalMemoryLimit:364->createQueryRunner:385 » Creation
[ERROR] TestMemoryManager.testQueryUserMemoryLimit:351->createQueryRunner:385 » Creation
[ERROR] TestMemoryManager.testReservedPoolDisabled:178->createQueryRunner:385 » Creation
[ERROR] TestMemoryManager.testResourceOverCommit:94->createQueryRunner:385 » Creation ...
[ERROR] TestAggregations>AbstractTestQueryFramework.init:84->lambda$new$0:23 » Creation
[ERROR] TestGracefulShutdown.testCoordinatorShutdown:112 » Creation Unable to create i...
[ERROR] TestGracefulShutdown.testShutdown:74 » Creation Unable to create injector, see...
[ERROR] TestMetadataManager.setUp:57 » Creation Unable to create injector, see the fol...
[ERROR] TestMetadataManager.tearDown:83 NullPointer
[ERROR] TestMinWorkerRequirement.testInitializationTimeout:82 » Creation Unable to cre...
[ERROR] TestMinWorkerRequirement.testInsufficientInitialWorkerNodes:34 » Creation Unab...
[ERROR] TestMinWorkerRequirement.testInsufficientInitialWorkerNodesWithCoordinatorExcluded:48 » Creation
[ERROR] TestMinWorkerRequirement.testInsufficientWorkerNodes:98 » Creation Unable to c...
[ERROR] TestMinWorkerRequirement.testInsufficientWorkerNodesAfterDrop:131 » Creation U...
[ERROR] TestMinWorkerRequirement.testInsufficientWorkerNodesWithCoordinatorExcluded:115 » Creation
[ERROR] TestMinWorkerRequirement.testSufficientInitialWorkerNodes:61 » Creation Unable...
[ERROR] TestOptimizeMixedDistinctAggregations>AbstractTestQueryFramework.init:84->lambda$new$0:25 » Creation
[ERROR] TestQueryManager.setUp:48 » Creation Unable to create injector, see the follow...
[ERROR] TestQueryManager.tearDown:54 NullPointer
[ERROR] TestTpchDistributedStats.setup:45 » Creation Unable to create injector, see th...
[ERROR] TestTpchDistributedStats.tearDown:56 NullPointer
Praveen2112
force-pushed
the
remove_datasources
branch
from
640c7b1
to
984d2ed
Compare
Praveen2112
changed the title
Praveen2112
force-pushed
the
remove_datasources
branch
3 times, most recently
from
0f0de10
to
9e0869a
Compare
kokosing
reviewed
Feb 25, 2019
| String property = nullToEmpty(announcement.getProperties().get("connectorIds")); | ||
| List<String> values = Splitter.on(',').trimResults().omitEmptyStrings().splitToList(property); | ||
| Set<String> connectorIds = new LinkedHashSet<>(values); | ||
| Set<String> connectorIds = new LinkedHashSet<>(); | ||
|
|
||
| // automatically build connectorIds if not configured | ||
| if (connectorIds.isEmpty()) { |
There was a problem hiding this comment.
connectorIds.isEmpty() is always true. Please try to remove connectorIds variable and optimize the if block.
electrum
changed the title
electrum
reviewed
Feb 25, 2019
| @@ -183,9 +178,7 @@ private static void updateConnectorIds(Announcer announcer, CatalogManager metad | |||
| // build announcement with updated sources | |||
| ServiceAnnouncementBuilder builder = serviceAnnouncement(announcement.getType()); | |||
| for (Map.Entry<String, String> entry : announcement.getProperties().entrySet()) { | |||
There was a problem hiding this comment.
We can now replace this loop with
builder.addProperties(announcement.getProperties());
electrum
approved these changes
Feb 25, 2019
Praveen2112
force-pushed
the
remove_datasources
branch
2 times, most recently
from
2bedf52
to
737f1a9
Compare
Praveen2112
force-pushed
the
remove_datasources
branch
from
737f1a9
to
4945840
Compare
rice668
pushed a commit
to rice668/trino
that referenced
this pull request
Nov 29, 2023
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.
Datasources property in ServerConfig is depricated and has not been in use for quite
a long time.