PSQL changes & naksha alignment #102
Conversation
| .withDefaultSchema(NakshaAdminCollection.SCHEMA) | ||
| .build(); | ||
| return new NakshaApp(config, cfgId, null); | ||
| final PsqlInstanceConfig config = |
There was a problem hiding this comment.
.parseUrl(url)
.withDefaultSchema(NakshaAdminCollection.SCHEMA)
Earlier code was ensuring that if schema is not provided as part of URL, then default schema naksha will be used. This allows us using different (custom) schema in the URL, even if using the same database instance.
Not sure if new change still supports this capability? But we surely need it.
There was a problem hiding this comment.
I am unaware of the reasons behind this change, we might readdress this after talking with Alex or Paweł about it
here-naksha-lib-core/src/main/java/com/here/naksha/lib/core/AbstractTask.java
Outdated
Show resolved
Hide resolved
| @@ -316,7 +429,7 @@ protected final void unlock() { | |||
|
|
|||
| private static final AtomicLong threadCount = new AtomicLong(); | |||
|
|
|||
| private @NotNull RESULT init_and_execute() { | |||
| private @NotNull RESULT init_and_execute() throws Exception { | |||
There was a problem hiding this comment.
avoid throwing exception, as this was the main reason we were seeing API requests hanging during pipeline execution.
There was a problem hiding this comment.
changed and fixed along with errorResult
| RESULT result = null; | ||
| log.warn("The task failed with an exception. ", throwable); | ||
| return result; | ||
| protected @NotNull RESULT errorResponse(@NotNull Throwable throwable) throws Exception { |
There was a problem hiding this comment.
Seems like the merge conflict is bringing back the old code. In the new change, function returns empty result and doesn't throw exception. This was added as a fix to ensure we don't get into API request hanging issue again we observed in pipeline.
There was a problem hiding this comment.
returned back to explicit null returning - though I think we need to tackle it with something else than null (maybe explicitly modelled error result?)
There was a problem hiding this comment.
This class has significant changes compared to what was refactored in maintenance branch. Need to understand this further. Leave this open for now. I will check with Alex / Pawel about the changes and what can be done.
There was a problem hiding this comment.
After having gone through all the changes, it looks like this has knock-on impact across files, if we have to make further changes in PluginCache. And it may not be worth blocking entire PR merge for the same. So, ignore my previous comment on this file and I am OK to go ahead with current changes.
There was a problem hiding this comment.
As this class is applicable only for Naksha internal Admin resources (like Storage, EventHandler, Space and in future Subscription), perhaps worth renaming it to AdminFeatureEventHandler or something similar , to not confuse it with Custom Space based XyzFeatures.
There was a problem hiding this comment.
nice, renamed + added minor javadoc
| } | ||
| return storage.newInstance(this); | ||
| final Storage storage = | ||
| Objects.requireNonNull(readFeatureFromResult(result, Storage.class), "Storage not found in result"); |
There was a problem hiding this comment.
eventhough minor thing, but let's retain the previous message (which prints the storageId)
("No storage found with id " + storageId)
Code Coverage
Files
|
No description provided.