[AlcorLib] Support Multi-Params Query #252

Gzure · 2020-06-16T11:53:04Z

This PR proposes the following changes:

Features:

change IgniteClient to Ignite node client
add support multi params query
HTTP response can according to URL query param [field] filter need entity fields
unit test can initialize the Ignite client bean on demand
replace webflux with spring gateway route define
support default value return for HTTP JSON response
add a new API [/v2.0/extensions] in API GW
add security group API in API GW
support return both project_id and tenant_id

Bugs:

fix VPC , Subnet, Port microservices return value not correct
move generate UUID from API GW to microservice
remove some no need check in VPC and Subnet microservices

xieus · 2020-06-16T12:02:54Z

@Gzure Thanks for submitting the PR 👍

Quick comment: can you please take a look at the test result shown on https://github.com/futurewei-cloud/alcor/pull/252/checks?check_run_id=776341456? It seems that some existing Ignite UTs in AlcorLib doesn't pass. Maybe a UT issue.

xieus · 2020-06-16T12:09:11Z

lib/src/main/java/com/futurewei/alcor/common/db/ignite/IgniteCacheFactory.java

 public class IgniteCacheFactory implements ICacheFactory {

-    private IgniteClient igniteClient;
+    private Ignite ignite;


What is the difference of ignite.Ignite and ignite.client.IgniteClient? Is it Ignite some sort of client?

@Gzure Could you think of renaming the class "ignite.Ignite" with more specific meaning, for example, IgniteXYZClient?

Ignite.client.IgniteClient is a thin ignite client, about thin client https://www.gridgain.com/docs/latest/developers-guide/thin-clients/getting-started-with-thin-clients. A ignite. Ignite as an Ignite cluster node in client mode can support more functions than IgniteClient class, but it does not store data.

Okay. Thanks.

We might need to consider:

Slow client handling (ref: https://apacheignite.readme.io/docs/clients-vs-servers#managing-slow-clients).

Client reconnect handling (ref: https://apacheignite.readme.io/docs/clients-vs-servers#reconnecting-a-client). This requires adding a special exception handling in the catch block.

Number of Ignite nodes in client mode. If the number goes large, it could affect the performance on the server side.

xieus · 2020-06-20T06:46:39Z

@Gzure It seems that the latest alcor/master has some conflicting files. Could you do a pull and merge the conflict? Meanwhile, the PR history above shows some commits that were previously included in other PRs. Maybe you want to clean up your branch history a bit.

chenpiaoping · 2020-06-28T03:06:43Z

lib/src/main/java/com/futurewei/alcor/common/db/ignite/IgniteDbCache.java

        try {
-            cache = igniteClient.getOrCreateCache(name);
+            cache = ignite.getOrCreateCache(name);
        } catch (ClientException e) {


Since we have used Ignite instead of IgniteClient, the exceptions of cache operations should be modified accordingly.

chenpiaoping · 2020-06-28T03:30:16Z

lib/src/main/java/com/futurewei/alcor/common/db/ignite/IgniteDbCache.java

+            return null;
+        }
+
+        E2 obj = result.get(0).getValue();


How to ensure that there is only one result of the query? if so, is the getAll (CachePredicate < E1, E2 > cachePredicate) interface sufficient?

chenpiaoping · 2020-06-28T03:32:07Z

lib/src/main/java/com/futurewei/alcor/common/db/query/impl/MapPredicate.java

+import com.futurewei.alcor.common.db.query.CachePredicate;
+import com.google.common.collect.ImmutableMap;
+import org.apache.ignite.binary.BinaryObject;
+import org.apache.ignite.cache.query.ScanQuery;


Let's delete useless import.

Gzure · 2020-06-29T08:12:06Z

@xieus Based on the test results with OpenStack, some modifications were made. Now two microservices of VPC and Subnet can be integrated with OpenStack normally.

xieus · 2020-06-30T01:56:17Z

@xieus Based on the test results with OpenStack, some modifications were made. Now two microservices of VPC and Subnet can be integrated with OpenStack normally.

This is very cool! Thanks, @Gzure. Could you write a short instruction on Alcor-Nova integration including how to register Alcor on a running Nova service, how to trigger testing from Nova to Alcor etc.

Gzure · 2020-06-30T02:40:29Z

@xieus All we need to do is according integration_nova doc [How nova client identify alcor server url] section to register Alcor on OpenStack (include nova).

xieus · 2020-06-30T04:45:07Z

lib/src/main/java/com/futurewei/alcor/common/db/ICache.java

+    /**
+     * Get Cache value from cache db by multi params
+     *
+     * @param var1 the cache key


Seems that we need to update the comments here.

xieus · 2020-06-30T04:49:10Z

lib/src/test/java/com/futurewei/alcor/common/cache/ControllerUtilTest.java

+public class ControllerUtilTest {
+
+    @Test
+    public void transformUrlPathParamsTest() throws QueryParamTypeNotSupportException {


xieus · 2020-06-30T04:50:34Z

...s/api_gateway/src/main/java/com/futurewei/alcor/apigateway/filter/KeystoneConfiguration.java

+    //@Bean
    public KeystoneAuthWebFilter keystoneAuthWebFilter(){
        if(!keystoneEnable){
            return null;
        }
        return new KeystoneAuthWebFilter();
    }

+    @Bean
+    public KeystoneAuthGwFilter keystoneAuthGwFilter(){
+        if(!keystoneEnable){
+            return null;
+        }
+        return new KeystoneAuthGwFilter();
+    }


duplicated issue or merge issue?

They are different filter. KeystoneAuthGwFilter is used for Spring Gateway. KeystoneAuthWebFilter is used for Webflux filter.
Since webflux is now replaced with spring gateway, KeystoneAuthWebFilteris no longer useful。

I see. Thanks for the clarification.

xieus · 2020-06-30T04:55:44Z

LGTM. @chenpiaoping Could you also continue to review and sign off the PR when it is ready?

xieus · 2020-06-30T04:56:10Z

@xieus All we need to do is according integration_nova doc [How nova client identify alcor server url] section to register Alcor on OpenStack (include nova).

Good to know. Thanks for confirmation.

xieus · 2020-06-30T04:57:29Z

This PR proposes a critical fundamental features as well as a few bug fix. Please list them all in the PR description. @Gzure

chenpiaoping · 2020-06-30T06:12:42Z

LGTM. @chenpiaoping Could you also continue to review and sign off the PR when it is ready?

sure

Gzure · 2020-06-30T06:51:13Z

This PR proposes a critical fundamental features as well as a few bug fix. Please list them all in the PR description. @Gzure

ok

lib/src/main/java/com/futurewei/alcor/common/db/query/ScanQueryBuilder.java

lib/src/main/java/com/futurewei/alcor/common/db/query/impl/MapPredicate.java

lib/src/main/java/com/futurewei/alcor/common/db/ignite/IgniteDbCache.java

chenpiaoping · 2020-07-02T06:45:08Z

lib/src/main/java/com/futurewei/alcor/common/db/ignite/IgniteTransaction.java

    }

+    @Override
    public Transaction start() throws CacheException {


Do we still need throws CacheException here?

IgniteClienTransaction need

chenpiaoping · 2020-07-02T06:46:25Z

lib/src/main/java/com/futurewei/alcor/common/db/ignite/IgniteTransaction.java

-        if (clientTransaction != null) {
-            clientTransaction.close();
+        if (transaction != null) {
+            transaction.close();


May need to catch IgniteException here.

lib/src/main/java/com/futurewei/alcor/common/db/redis/RedisExpireCache.java

xieus · 2020-07-03T01:00:42Z

lib/src/main/java/com/futurewei/alcor/common/db/ignite/IgniteTransaction.java


-    private IgniteClient igniteClient;
-    private ClientTransaction clientTransaction;
+    private final Ignite client;


@Gzure Should Ignitetransaction also support IgniteClient based on the setting?

This is non-blocking. If we do need that, we can do it next PR.

@Gzure Should Ignitetransaction also support IgniteClient based on the setting?

This is non-blocking. If we do need that, we can do it next PR.

I had added a IgniteClientTransaction which according to Ignite thin client

xieus · 2020-07-03T01:03:26Z

lib/src/main/java/com/futurewei/alcor/common/db/ignite/MockIgniteServer.java

-                ClientConnectorConfiguration clientConfig = new ClientConnectorConfiguration();
-                clientConfig.setPort(ListenPort);
-
                org.apache.ignite.configuration.IgniteConfiguration cfg = new org.apache.ignite.configuration.IgniteConfiguration();


In our MockIgniteServer, do we need to test both IgniteClient and Ignite?

xieus

This PR addresses one of the most critical needs in the Alcor project - multi-param query - with many other enhancement of feature and UT throughout the project.

Approve it with pleasure. Thank you for your contribution @Gzure

xieus · 2020-07-03T01:13:08Z

..._gateway/src/main/java/com/futurewei/alcor/apigateway/proxies/SubnetManagerServiceProxy.java

 import java.util.UUID;

-@Service
+//@Service


If not that in use anyway, we can consider deprecate then remove them

xieus · 2020-07-03T01:36:06Z

@Gzure Somehow merge from latest alcor/master doesn't work (failure in RouteManager likely due to a recent check-in). Could you take a quick look? Thanks.

add field filter support add UTs skip init ignite client bean

make some api return value meet neutron api

Gzure · 2020-07-03T02:42:13Z

@Gzure Somehow merge from latest alcor/master doesn't work (failure in RouteManager likely due to a recent check-in). Could you take a quick look? Thanks.

It confict with new Merge PR, Have fixed.

xieus self-requested a review June 16, 2020 11:58

xieus assigned Gzure Jun 16, 2020

xieus added the feature feature development label Jun 16, 2020

xieus added this to the Version 0.6.2020.06.30 milestone Jun 16, 2020

xieus linked an issue Jun 16, 2020 that may be closed by this pull request

Integration with Nova Compute (in the vm creation scenario) #190

Closed

xieus changed the title ~~Multi params query~~ [AlcorLib] Support Multi Params Query Jun 16, 2020

xieus reviewed Jun 17, 2020

View reviewed changes

Gzure force-pushed the multi-params-query branch from b23e780 to fa769da Compare June 22, 2020 02:58

xieus linked an issue Jun 23, 2020 that may be closed by this pull request

[Microservice Enhancement] APIs support multi params query #233

Closed

chenpiaoping reviewed Jun 28, 2020

View reviewed changes

Gzure force-pushed the multi-params-query branch from 2b213cc to 7bf67ab Compare June 29, 2020 07:50

xieus reviewed Jun 30, 2020

View reviewed changes

chenpiaoping reviewed Jul 1, 2020

View reviewed changes

Gzure force-pushed the multi-params-query branch from 79ce34e to c14d746 Compare July 2, 2020 03:44

chenpiaoping reviewed Jul 2, 2020

View reviewed changes

chenpiaoping approved these changes Jul 2, 2020

View reviewed changes

xieus reviewed Jul 3, 2020

View reviewed changes

xieus approved these changes Jul 3, 2020

View reviewed changes

Gzure added 16 commits July 3, 2020 10:12

add multi params query support

90d19e4

add field filter support add UTs skip init ignite client bean

add handle default value handle

f02e022

rename dhcp_enable to enable_dhcp

a9da252

remove some check in update subnet

ddc3f66

change return type to PortWebBulkJson

c2ee773

change return type to VpcsWebJson

35a0b2f

change vpcs to networks

9f59fcc

change webflux to spring gateway and add port route defines

a43bd28

change ClientException to IgniteException

0044a1f

rewrite test unit and remove some beans

60cd4df

fix vpc and subnet create post problem

f142662

make some api return value meet neutron api

add securityGroup route in API Gateway

2f81529

support return both project_id and tenant_id

1df69df

add ignite thin client and rewrite some db interfaces

2296ca2

add new interface IgniteICache extends ICache

ca8a57a

add some Exception handle in ignite Transaction class

bc42ef2

Gzure force-pushed the multi-params-query branch from 9d8ed7e to bc42ef2 Compare July 3, 2020 02:30

merge with master

0af4b61

xieus changed the title ~~[AlcorLib] Support Multi Params Query~~ [AlcorLib] Support Multi-Params Query Jul 3, 2020

xieus merged commit 0276c27 into futurewei-cloud:master Jul 3, 2020

Gzure deleted the multi-params-query branch July 3, 2020 08:18

[AlcorLib] Support Multi-Params Query #252

[AlcorLib] Support Multi-Params Query #252

Uh oh!

Conversation

Gzure commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xieus commented Jun 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xieus commented Jun 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gzure commented Jun 29, 2020

Uh oh!

xieus commented Jun 30, 2020

Uh oh!

Gzure commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xieus commented Jun 30, 2020

Uh oh!

xieus commented Jun 30, 2020

Uh oh!

xieus commented Jun 30, 2020

Uh oh!

chenpiaoping commented Jun 30, 2020

Uh oh!

Gzure commented Jun 30, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xieus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xieus commented Jul 3, 2020

Uh oh!

Gzure commented Jul 3, 2020

Uh oh!

Reviewers

Gzure commented Jun 16, 2020 •

edited

Loading