Uploaded image for project: 'Falcon'
  1. Falcon
  2. FALCON-469 Enhance REST API to drive interactive user dashboard
  3. FALCON-470

Add support for pagination, filter by, etc. to Entity and Instance List API

Details

    • Sub-task
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • 0.6
    • 0.6
    • webapp

    Attachments

      1. Falcon-Jira-470-471-472-473-v1.patch
        101 kB
        Balu Vellanki
      2. Falcon-Jira-470-471-472-473-v3.patch
        149 kB
        Balu Vellanki
      3. Falcon-Jira-470-471-472-473-v4.patch
        149 kB
        Balu Vellanki
      4. Falcon-Jira-470-471-472-473-v5.patch
        153 kB
        Balu Vellanki
      5. Falcon-Jira-470-471-472-473-v6.patch
        123 kB
        Balu Vellanki

      Issue Links

        Activity

          bvellanki Balu Vellanki added a comment -

          Added support for pagination for list api. New query params introduced are
          – orderBy : String. Optional. Sort the list results by specific key. Supports orderBy name, status and entity type. By default, results are not sorted.
          – offset : int. Optional. Start showing results from Nth entity. Default is 1st entity.
          – count : int. Optional. Show N results per fetch. Default value is -1 (show all entities)

          bvellanki Balu Vellanki added a comment - Added support for pagination for list api. New query params introduced are – orderBy : String. Optional. Sort the list results by specific key. Supports orderBy name, status and entity type. By default, results are not sorted. – offset : int. Optional. Start showing results from Nth entity. Default is 1st entity. – count : int. Optional. Show N results per fetch. Default value is -1 (show all entities)

          Couple of things that needs to be taken care of before considering complete:

          • These newly added params should be exposed in the CLI - look at FalconClient and FalconCLI
          • Unit tests are missing
          • Add system tests for FalconCLIT and may be EntityManagerJerseyIT
          svenkat Venkatesh Seetharam added a comment - Couple of things that needs to be taken care of before considering complete: These newly added params should be exposed in the CLI - look at FalconClient and FalconCLI Unit tests are missing Add system tests for FalconCLIT and may be EntityManagerJerseyIT
          bvellanki Balu Vellanki added a comment -

          Working on CLI and tests. All code required for Falcon-471 and Falcon-472 will be part of this patch.

          Thank you

          bvellanki Balu Vellanki added a comment - Working on CLI and tests. All code required for Falcon-471 and Falcon-472 will be part of this patch. Thank you

          Please also make sure to update the docs in docs module for the API changes.

          svenkat Venkatesh Seetharam added a comment - Please also make sure to update the docs in docs module for the API changes.
          bvellanki Balu Vellanki added a comment -

          new patch available. This patch handles following.

          • Necessary code changes for Jira 470, 471, 472, 473
          • Added integration tests
          • Updated RestAPI documentation
          bvellanki Balu Vellanki added a comment - new patch available. This patch handles following. Necessary code changes for Jira 470, 471, 472, 473 Added integration tests Updated RestAPI documentation
          bvellanki Balu Vellanki added a comment -

          Canceling old patch so that I can submit a new and cleaner patch

          bvellanki Balu Vellanki added a comment - Canceling old patch so that I can submit a new and cleaner patch
          bvellanki Balu Vellanki added a comment -

          This version of patch added unit tests that can handle implicit user filter.

          bvellanki Balu Vellanki added a comment - This version of patch added unit tests that can handle implicit user filter.

          A lot of issues with the code and it wont compile.

          FalconCLI

          • lot of repetitive code, can refactor into a method
                    if (commandLine.getOptionValue(OFFSET_OPT) != null) {
                        try {
                            offset = Integer.parseInt(commandLine.getOptionValue(OFFSET_OPT));
                        } catch (NumberFormatException e) {
                            throw new FalconCLIException("Input value provided for queryParam \"offset\" is not a valid Integer");
                        }
                    }
            
          • refactor this into a method
                        if (fields == null) {
                            fields = "";
                        } else if (!fields.equalsIgnoreCase("status")) {
                            throw new FalconCLIException("Invalid value for queryParam \"fields\" : "+fields);
                        }
            
          • CLI is getting too unweildy, we could may be add enums and construct options from 'em, may be another jira

          FalconClient

          • why have you removed SUSPEND CHECKSTYLE CHECK ParameterNumberCheck
          • Formatting is incorrect in ResourceList.twiki. Did you verify the generated HTML?
          • You have disabled a test: org/apache/falcon/resource/EntityManagerJerseyIT.testProcessInputUpdate
          • ProcessInstanceManagerIT is disabled, how did you verify?

          AbstractEntityManager

          • why do you have ugi as an instance variable?
            protected UserGroupInformation currentUgi
          • getEntityList is quite long, bad practice, pls refactor into methods.
            filterByField init is a easy candidate
          • There are 2 duplicate methods for isEntityAuthorized

          AbstractInstanceManager

          • method getInstanceResultSubset is too long, refactor
          • I see a log of repetitive code which can be factored into the base class. pls apply DRY principle
          svenkat Venkatesh Seetharam added a comment - A lot of issues with the code and it wont compile. FalconCLI lot of repetitive code, can refactor into a method if (commandLine.getOptionValue(OFFSET_OPT) != null ) { try { offset = Integer .parseInt(commandLine.getOptionValue(OFFSET_OPT)); } catch (NumberFormatException e) { throw new FalconCLIException( "Input value provided for queryParam \" offset\ " is not a valid Integer " ); } } refactor this into a method if (fields == null ) { fields = ""; } else if (!fields.equalsIgnoreCase( "status" )) { throw new FalconCLIException( "Invalid value for queryParam \" fields\ " : " +fields); } CLI is getting too unweildy, we could may be add enums and construct options from 'em, may be another jira FalconClient why have you removed SUSPEND CHECKSTYLE CHECK ParameterNumberCheck Formatting is incorrect in ResourceList.twiki. Did you verify the generated HTML? You have disabled a test: org/apache/falcon/resource/EntityManagerJerseyIT.testProcessInputUpdate ProcessInstanceManagerIT is disabled, how did you verify? AbstractEntityManager why do you have ugi as an instance variable? protected UserGroupInformation currentUgi getEntityList is quite long, bad practice, pls refactor into methods. filterByField init is a easy candidate There are 2 duplicate methods for isEntityAuthorized AbstractInstanceManager method getInstanceResultSubset is too long, refactor I see a log of repetitive code which can be factored into the base class. pls apply DRY principle
          bvellanki Balu Vellanki added a comment -

          Thank you Venkatesh. I followed your suggestions for ResourceList.twiki, AbstractEntityManager, AbstractInstanceManager and FalconCLI.

          FalconCLI is becoming unwieldy and we should tackle it in a separate JIRA ticket.

          FalconClient - I did not remove the parameterCheck, I moved it to encompass more methods.

          EntityManagerJerseyIT - The test was disabled by mistake, I enabled it again.

          ProcessInstanceManagerIT - The tests have been disabled in the source as they take too long. For the sake of testing, I enabled them, ran the tests and disabled again once all the tests passed.

          bvellanki Balu Vellanki added a comment - Thank you Venkatesh. I followed your suggestions for ResourceList.twiki, AbstractEntityManager, AbstractInstanceManager and FalconCLI. FalconCLI is becoming unwieldy and we should tackle it in a separate JIRA ticket. FalconClient - I did not remove the parameterCheck, I moved it to encompass more methods. EntityManagerJerseyIT - The test was disabled by mistake, I enabled it again. ProcessInstanceManagerIT - The tests have been disabled in the source as they take too long. For the sake of testing, I enabled them, ran the tests and disabled again once all the tests passed.
          bvellanki Balu Vellanki added a comment -

          Patch for all sub-tasks under JIRA-469

          bvellanki Balu Vellanki added a comment - Patch for all sub-tasks under JIRA-469
          sowmyaramesh Sowmya Ramesh added a comment -

          1. Please remove protected EntityList getEntityList in AbstractEntityManager and call public method from tests
          2. AbstractEntityManager line 504: configStore.get can return null. Can we do null validation before accessing it?
          3. Minor nit: In previous version entityTypeString = type.toLowerCase(). Can you retain this behavior?
          4. AbstractEntityManager.getStatusString : null check at 606 is redundant since enum.name can never return null. Also please change the caught exception from throwable to FalconException
          5. nit: Lot of places you check the condition and then do ! e.g. Instead of if (!(filterTagsList.size() == 0)) , please use != 0
          6. nit: Few util methods can be made static and params final
          7. AbstractEntityManager Line 623 can filterValue be null?
          8. AbstractEntityManager : Move try IllegalArgumentException to line 638
          9. Can you rename getReturnArrayLength to getRequiredNumberOfResults
          10. FalconCLI: Validate methods ideally should be void methods doing validation and shouldn't return any value. Can you please change it?
          11. FalconClient : Why was import changed to *? Please import only required classes.

          Thanks!

          sowmyaramesh Sowmya Ramesh added a comment - 1. Please remove protected EntityList getEntityList in AbstractEntityManager and call public method from tests 2. AbstractEntityManager line 504: configStore.get can return null. Can we do null validation before accessing it? 3. Minor nit: In previous version entityTypeString = type.toLowerCase(). Can you retain this behavior? 4. AbstractEntityManager.getStatusString : null check at 606 is redundant since enum.name can never return null. Also please change the caught exception from throwable to FalconException 5. nit: Lot of places you check the condition and then do ! e.g. Instead of if (!(filterTagsList.size() == 0)) , please use != 0 6. nit: Few util methods can be made static and params final 7. AbstractEntityManager Line 623 can filterValue be null? 8. AbstractEntityManager : Move try IllegalArgumentException to line 638 9. Can you rename getReturnArrayLength to getRequiredNumberOfResults 10. FalconCLI: Validate methods ideally should be void methods doing validation and shouldn't return any value. Can you please change it? 11. FalconClient : Why was import changed to *? Please import only required classes. Thanks!
          bvellanki Balu Vellanki added a comment -

          Thank you sowmyaramesh for the suggestions. I agree with and made all the changes you recommended.

          bvellanki Balu Vellanki added a comment - Thank you sowmyaramesh for the suggestions. I agree with and made all the changes you recommended.

          bvellanki, few minor nits:

          • FalconCLI
            Why is this necessary. Pls pass null.
                   if (orderBy == null) {
                        orderBy = "";
                    }
                    if (filterBy == null) {
                        filterBy = "";
                    }
                    if (instanceOrderBy == null) {
                        instanceOrderBy = "";
                    }
                    if (instanceFilter == null) {
                        instanceFilter = "";
                    }
                    if (fields == null) {
                        fields = "";
                    }
            
          • FalconClient
            if (filterBy != null && !filterBy.equals("")) {

            Use StringUtils.isEmpty instead

          • I do not see unit tests for pipelines. BTW, I thought you'd have a separate patch for pipeline element for process.
          svenkat Venkatesh Seetharam added a comment - bvellanki , few minor nits: FalconCLI Why is this necessary. Pls pass null. if (orderBy == null ) { orderBy = ""; } if (filterBy == null ) { filterBy = ""; } if (instanceOrderBy == null ) { instanceOrderBy = ""; } if (instanceFilter == null ) { instanceFilter = ""; } if (fields == null ) { fields = ""; } FalconClient if (filterBy != null && !filterBy.equals("")) { Use StringUtils.isEmpty instead I do not see unit tests for pipelines. BTW, I thought you'd have a separate patch for pipeline element for process.
          bvellanki Balu Vellanki added a comment -
          • I am making changes as per your suggestion for FalconCLI and FalconClient.
          • I had integration tests for Pipeline. I am adding unit tests now. The code for Pipeline is touching the same files as the rest of the patch for this JIRA, hence I added the fix for pipeline here. I understand this is not something we should do in the future.
          bvellanki Balu Vellanki added a comment - I am making changes as per your suggestion for FalconCLI and FalconClient. I had integration tests for Pipeline. I am adding unit tests now. The code for Pipeline is touching the same files as the rest of the patch for this JIRA, hence I added the fix for pipeline here. I understand this is not something we should do in the future.

          Thanks bvellanki for your contribution.

          svenkat Venkatesh Seetharam added a comment - Thanks bvellanki for your contribution.

          People

            bvellanki Balu Vellanki
            svenkat Venkatesh Seetharam
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: